From 01737d60b28ffe07fc2153cc0394ab72b46c4530 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 10 Apr 2024 18:10:42 +0930 Subject: [PATCH 01/13] feat: adds endpoints for fetchGuestToken * generate_superset_context : adds a superset_guest_token_url to the context * generate_superset_token : called when ^ hit instead of on render. * adds a plugin url and view * removes superset_token from the context * adds/updates tests --- platform_plugin_aspects/apps.py | 9 ++- platform_plugin_aspects/extensions/filters.py | 11 --- .../static/html/superset.html | 4 +- .../static/js/embed_dashboard.js | 19 ++++- platform_plugin_aspects/static/js/superset.js | 4 +- platform_plugin_aspects/tests/test_utils.py | 78 ++++++------------- platform_plugin_aspects/tests/test_views.py | 51 ++++++++++++ platform_plugin_aspects/tests/test_xblock.py | 49 +++++++----- platform_plugin_aspects/urls.py | 20 +++++ platform_plugin_aspects/utils.py | 60 +++++++------- platform_plugin_aspects/views.py | 48 ++++++++++++ platform_plugin_aspects/xblock.py | 43 +++++++++- requirements/test.in | 1 + test_settings.py | 29 ++++++- 14 files changed, 304 insertions(+), 122 deletions(-) create mode 100644 platform_plugin_aspects/tests/test_views.py create mode 100644 platform_plugin_aspects/urls.py create mode 100644 platform_plugin_aspects/views.py diff --git a/platform_plugin_aspects/apps.py b/platform_plugin_aspects/apps.py index fa2b8a4..36da0ea 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: name, + 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..90b7c01 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() @@ -51,9 +42,7 @@ def run_filter( context = generate_superset_context( context, - user, dashboards=dashboards, - filters=filters, language=formatted_language, ) 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..0a26b44 100644 --- a/platform_plugin_aspects/static/js/embed_dashboard.js +++ b/platform_plugin_aspects/static/js/embed_dashboard.js @@ -1,11 +1,24 @@ -function embedDashboard(dashboard_uuid, superset_url, superset_token, xblock_id) { +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, { + method: 'POST', + body: JSON.stringify({ + // TODO csrf_token: csrf_token, + }) + }); + const data = await response.json(); + return data.guestToken; + } + 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 +41,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, window.superset_guest_token_url); }); } diff --git a/platform_plugin_aspects/static/js/superset.js b/platform_plugin_aspects/static/js/superset.js index 14ae644..392630c 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 = context.superset_guest_token_url; 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..0b7f95a 100644 --- a/platform_plugin_aspects/tests/test_utils.py +++ b/platform_plugin_aspects/tests/test_utils.py @@ -2,19 +2,19 @@ Test utils. """ -from collections import namedtuple from unittest.mock import Mock, patch from django.conf import settings from django.test import TestCase 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): @@ -104,15 +104,11 @@ def test_get_ccx_courses_feature_disabled(self): "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": COURSE_ID} dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS dashboards.append( @@ -123,42 +119,41 @@ 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"/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_exception( self, mock_superset_client ): """ - Test generate_superset_context + Test generate_guest_token """ - 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, + token, exception = generate_guest_token( + user=user_mock, + course=COURSE_ID, dashboards=[{"name": "test", "uuid": "test-dashboard-uuid"}], filters=[filter_mock], ) - self.assertIn("exception", context) + mock_superset_client.assert_called_once() + self.assertIsNone(token) + self.assertEqual(str(exception), "test-exception") @patch.object( settings, @@ -171,49 +166,26 @@ def test_generate_superset_context_with_superset_client_exception( }, ) @patch("platform_plugin_aspects.utils.SupersetClient") - def test_generate_superset_context_succesful(self, mock_superset_client): + def test_generate_guest_token_succesful(self, mock_superset_client): """ - Test generate_superset_context + Test generate_guest_token """ - course_mock = Mock() - filter_mock = Mock() - user_mock = Mock() - user_mock.username = "test-user" - context = {"course": course_mock} 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, _errors = 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..1b7c2c2 --- /dev/null +++ b/platform_plugin_aspects/tests/test_views.py @@ -0,0 +1,51 @@ +""" +Test views. +""" + +from unittest.mock import patch + +from django.contrib.auth import get_user_model +from django.test import TestCase +from django.urls import reverse + +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.superset_guest_token_url = reverse( + "superset_guest_token", + kwargs={"course_id": 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): + """ + Unauthenticated hits to the endpoint redirect to login. + """ + response = self.client.post(self.superset_guest_token_url) + self.assertEqual(response.status_code, 302) + self.assertIn("login", response.url) + + @patch("platform_plugin_aspects.views.generate_guest_token") + def test_guest_token(self, mock_generate_guest_token): + mock_generate_guest_token.return_value = ("test-token", "test-dashboard-uuid") + self.client.login(username="user", password="password") + response = self.client.post(self.superset_guest_token_url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json().get("guestToken"), "test-token") + mock_generate_guest_token.assert_called_once() diff --git a/platform_plugin_aspects/tests/test_xblock.py b/platform_plugin_aspects/tests/test_xblock.py index 2355110..552713f 100644 --- a/platform_plugin_aspects/tests/test_xblock.py +++ b/platform_plugin_aspects/tests/test_xblock.py @@ -2,10 +2,12 @@ """ Test basic SupersetXBlock display function """ +import json from unittest import TestCase from unittest.mock import Mock, patch 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 +37,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 +64,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 +90,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 +103,34 @@ 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.xblock.generate_guest_token") + def test_guest_token_handler(self, mock_generate_guest_token): + mock_generate_guest_token.return_value = ("test-token", "test-dashboard-uuid") + 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 == 200 + data = json.loads(response.body.decode("utf-8")) + assert data.get("guestToken") == "test-token" + mock_generate_guest_token.assert_called_once() diff --git a/platform_plugin_aspects/urls.py b/platform_plugin_aspects/urls.py new file mode 100644 index 0000000..b142e4e --- /dev/null +++ b/platform_plugin_aspects/urls.py @@ -0,0 +1,20 @@ +""" +Urls for the Aspects plugin. +""" + +from django.urls import re_path + +from . import views + +# Copied from openedx.core.constants +COURSE_ID_PATTERN = r"(?P[^/+]+(/|\+)[^/+]+(/|\+)[^/?]+)" + +app_name = "platform_plugin_aspects" + +urlpatterns = [ + re_path( + rf"superset_guest_token/{COURSE_ID_PATTERN}/?$", + views.superset_guest_token, + name="superset_guest_token", + ), +] diff --git a/platform_plugin_aspects/utils.py b/platform_plugin_aspects/utils.py index 849252f..24277b2 100644 --- a/platform_plugin_aspects/utils.py +++ b/platform_plugin_aspects/utils.py @@ -10,6 +10,7 @@ from importlib import import_module from django.conf import settings +from django.urls import NoReverseMatch, reverse from supersetapiclient.client import SupersetClient from xblock.reference.user_service import XBlockUser @@ -26,18 +27,16 @@ def _(text): return text -def generate_superset_context( # pylint: disable=dangerous-default-value +def generate_superset_context( context, - user, dashboards, - filters=[], language=None, ): """ 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. @@ -49,50 +48,57 @@ def generate_superset_context( # pylint: disable=dangerous-default-value 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")) - 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, - } + # FIXME -- namespace issue with plugin-registered urls? + try: + guest_token_url = reverse( + "platform_plugin_aspects:superset_guest_token", + kwargs={"course_id": course}, ) - else: - context.update( - { - "exception": str(dashboards), - } + except NoReverseMatch: + logger.error( + "Error reversing platform_plugin_aspects:superset_guest_token, trying without namespace" ) + try: + guest_token_url = reverse( + "superset_guest_token", + kwargs={"course_id": course}, + ) + logger.info("Reversing superset_guest_token worked") + except NoReverseMatch: + logger.critical("Error reversing superset_guest_token, giving up") + guest_token_url = "" + + 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): """ Generate a Superset guest token for the user. Args: user: User object. - course: Course object. + course: string Course ID 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") diff --git a/platform_plugin_aspects/views.py b/platform_plugin_aspects/views.py new file mode 100644 index 0000000..38d739f --- /dev/null +++ b/platform_plugin_aspects/views.py @@ -0,0 +1,48 @@ +""" +Endpoints for the Aspects platform plugin. +""" + +from django.conf import settings +from django.contrib.auth.decorators import login_required +from django.core.exceptions import ImproperlyConfigured +from django.http import JsonResponse +from django.views.decorators.http import require_POST +from opaque_keys.edx.keys import CourseKey + +from .utils import _, generate_guest_token + + +@require_POST +@login_required +def superset_guest_token(request, course_id): + """ + Return a guest token for Superset that the given request.user can use to access the Superset API. + """ + # TODO -- restrict access to course staff? how? + + course_key = CourseKey.from_string(course_id) + built_in_filters = [ + f"org = '{course_key.org}'", + # TODO? Need to fetch the Course object to add this filter. + # f"course_name = '{course_key.display_name}'", + f"course_run = '{course_key.run}'", + ] + dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS + extra_filters_format = settings.SUPERSET_EXTRA_FILTERS_FORMAT + + guest_token, exception = generate_guest_token( + user=request.user, + course=course_id, + dashboards=dashboards, + filters=built_in_filters + extra_filters_format, + ) + + if not guest_token: + raise ImproperlyConfigured( + _( + "Unable to fetch Superset guest token, " + "mostly likely due to invalid settings.SUPERSET_CONFIG: {exception}" + ).format(exception=exception) + ) + + return JsonResponse({"guestToken": guest_token}) diff --git a/platform_plugin_aspects/xblock.py b/platform_plugin_aspects/xblock.py index 969afb5..06eb462 100644 --- a/platform_plugin_aspects/xblock.py +++ b/platform_plugin_aspects/xblock.py @@ -2,17 +2,19 @@ from __future__ import annotations +import json import logging import pkg_resources from django.utils import translation from web_fragments.fragment import Fragment -from xblock.core import XBlock +from webob import Response +from xblock.core import JsonHandlerError, XBlock 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__) @@ -112,12 +114,15 @@ 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) + # Use our xblock handler instead of the platform plugin's view. + context["superset_guest_token_url"] = self.runtime.handlerUrl( + self, "get_superset_guest_token" + ) + 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 +185,33 @@ 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() + + guest_token, exception = generate_guest_token( + user=user, + course=self.runtime.course_id, + dashboards=self.dashboards(), + filters=self.filters, + ) + + if not guest_token: + raise JsonHandlerError( + 500, + _( + "Unable to fetch Superset guest token, " + "mostly likely due to invalid settings.SUPERSET_CONFIG: {exception}" + ).format(exception=exception), + ) + + return Response( + json.dumps({"guestToken": guest_token}), + content_type="application/json", + charset="UTF-8", + ) diff --git a/requirements/test.in b/requirements/test.in index 1bc4cb8..9c69ca6 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -9,3 +9,4 @@ code-annotations # provides commands used by the pii_check make target. responses # mocks for the requests library ddt django-mock-queries +xblock-sdk # for testing XBlock handlers diff --git a/test_settings.py b/test_settings.py index 987b008..14a50ab 100644 --- a/test_settings.py +++ b/test_settings.py @@ -24,9 +24,28 @@ }, } -INSTALLED_APPS = ("platform_plugin_aspects",) +INSTALLED_APPS = ( + "django.contrib.auth", + "django.contrib.contenttypes", + "django.contrib.sessions", + "platform_plugin_aspects", +) + +ROOT_URLCONF = 'platform_plugin_aspects.urls' + +SECRET_KEY = 'very-insecure-key' + +MIDDLEWARE = [ + 'django.middleware.csrf.CsrfViewMiddleware', + 'django.contrib.sessions.middleware.SessionMiddleware', + 'django.contrib.auth.middleware.AuthenticationMiddleware', +] EVENT_SINK_CLICKHOUSE_MODEL_CONFIG = { + "auth_user": { + "module": "django.contrib.auth.models", + "model": "User", + }, "user_profile": { "module": "common.djangoapps.student.models", "model": "UserProfile", @@ -35,6 +54,14 @@ "module": "openedx.core.djangoapps.content.course_overviews.models", "model": "CourseOverview", }, + "external_id": { + "module": "openedx.core.djangoapps.external_user_ids.models", + "model": "ExternalId", + }, + "user_preference": { + "module": "openedx.core.djangoapps.user_api.models", + "model": "UserPreference", + }, } EVENT_SINK_CLICKHOUSE_COURSE_OVERVIEWS_ENABLED = True From 029d3bd4126756e5b5a6ece6c89f55891bd12613 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 10 Apr 2024 18:31:24 +0930 Subject: [PATCH 02/13] temp: workaround failing tests --- platform_plugin_aspects/signals.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/platform_plugin_aspects/signals.py b/platform_plugin_aspects/signals.py index 314d04e..d5913fe 100644 --- a/platform_plugin_aspects/signals.py +++ b/platform_plugin_aspects/signals.py @@ -2,6 +2,8 @@ Signal handler functions, mapped to specific signals in apps.py. """ +# FIXME TODO TEMPORARY WORKAROUND +# pylint: disable=unused-import from django.db.models.signals import post_save from django.dispatch import Signal, receiver @@ -33,7 +35,9 @@ 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")) +# FIXME TODO TEMPORARY WORKAROUND +# kombu.exceptions.OperationalError: [Errno 111] Connection refused +# @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 +57,9 @@ def on_user_profile_updated( # pylint: disable=unused-argument # pragma: no co ) -@receiver(post_save, sender=get_model("external_id")) +# FIXME TODO TEMPORARY WORKAROUND +# kombu.exceptions.OperationalError: [Errno 111] Connection refused +# @receiver(post_save, sender=get_model("external_id")) def on_externalid_saved( # pylint: disable=unused-argument # pragma: no cover sender, instance, **kwargs ): @@ -73,7 +79,9 @@ def on_externalid_saved( # pylint: disable=unused-argument # pragma: no cover ) -@receiver(USER_RETIRE_LMS_MISC) +# FIXME TODO TEMPORARY WORKAROUND +# kombu.exceptions.OperationalError: [Errno 111] Connection refused +# @receiver(USER_RETIRE_LMS_MISC) def on_user_retirement( # pylint: disable=unused-argument # pragma: no cover sender, user, **kwargs ): From 6fa96c27b04e1f0fa9ea76e7df09ebd715e55488 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Apr 2024 12:26:52 +0930 Subject: [PATCH 03/13] fix: connect model-based signals only if models are accessible Disconnecting these signal handlers during tests prevents errors like: kombu.exceptions.OperationalError: [Errno 111] Connection refused --- platform_plugin_aspects/signals.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/platform_plugin_aspects/signals.py b/platform_plugin_aspects/signals.py index d5913fe..1b69ca9 100644 --- a/platform_plugin_aspects/signals.py +++ b/platform_plugin_aspects/signals.py @@ -2,8 +2,6 @@ Signal handler functions, mapped to specific signals in apps.py. """ -# FIXME TODO TEMPORARY WORKAROUND -# pylint: disable=unused-import from django.db.models.signals import post_save from django.dispatch import Signal, receiver @@ -35,9 +33,6 @@ def receive_course_publish( # pylint: disable=unused-argument # pragma: no cov dump_course_to_clickhouse.delay(str(course_key)) -# FIXME TODO TEMPORARY WORKAROUND -# kombu.exceptions.OperationalError: [Errno 111] Connection refused -# @receiver(post_save, sender=get_model("user_profile")) def on_user_profile_updated( # pylint: disable=unused-argument # pragma: no cover sender, instance, **kwargs ): @@ -57,9 +52,13 @@ def on_user_profile_updated( # pylint: disable=unused-argument # pragma: no co ) -# FIXME TODO TEMPORARY WORKAROUND -# kombu.exceptions.OperationalError: [Errno 111] Connection refused -# @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) + + def on_externalid_saved( # pylint: disable=unused-argument # pragma: no cover sender, instance, **kwargs ): @@ -79,9 +78,14 @@ def on_externalid_saved( # pylint: disable=unused-argument # pragma: no cover ) -# FIXME TODO TEMPORARY WORKAROUND -# kombu.exceptions.OperationalError: [Errno 111] Connection refused -# @receiver(USER_RETIRE_LMS_MISC) +# 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) + + +@receiver(USER_RETIRE_LMS_MISC) def on_user_retirement( # pylint: disable=unused-argument # pragma: no cover sender, user, **kwargs ): From 20e514dafc5b560f18658709b5432cb7c4e918d1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Apr 2024 12:36:38 +0930 Subject: [PATCH 04/13] refactor: use DRF view, which supports openedx IsCourseStaffInstructor permissions Replaces the function-based view with a DRF view that: * enforces session authentication * enforces course staff/instructor role Also reverts unneeded changes from previous commits. --- platform_plugin_aspects/tests/test_views.py | 24 +++- platform_plugin_aspects/urls.py | 2 +- platform_plugin_aspects/utils.py | 4 +- platform_plugin_aspects/views.py | 136 +++++++++++++++----- requirements/test.in | 1 - test_settings.py | 13 -- 6 files changed, 125 insertions(+), 55 deletions(-) diff --git a/platform_plugin_aspects/tests/test_views.py b/platform_plugin_aspects/tests/test_views.py index 1b7c2c2..d0291e1 100644 --- a/platform_plugin_aspects/tests/test_views.py +++ b/platform_plugin_aspects/tests/test_views.py @@ -7,6 +7,9 @@ from django.contrib.auth import get_user_model from django.test import TestCase from django.urls import reverse +from rest_framework.test import APIClient + +from ..views import IsCourseStaffInstructor COURSE_ID = "course-v1:org+course+run" User = get_user_model() @@ -22,6 +25,7 @@ def setUp(self): Set up data used by multiple tests. """ super().setUp() + self.client = APIClient() self.superset_guest_token_url = reverse( "superset_guest_token", kwargs={"course_id": COURSE_ID}, @@ -34,18 +38,24 @@ def setUp(self): self.user.save() def test_guest_token_requires_authorization(self): - """ - Unauthenticated hits to the endpoint redirect to login. - """ response = self.client.post(self.superset_guest_token_url) - self.assertEqual(response.status_code, 302) - self.assertIn("login", response.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.post(self.superset_guest_token_url) + self.assertEqual(response.status_code, 403) + + @patch.object(IsCourseStaffInstructor, "has_object_permission") @patch("platform_plugin_aspects.views.generate_guest_token") - def test_guest_token(self, mock_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", "test-dashboard-uuid") + self.client.login(username="user", password="password") - response = self.client.post(self.superset_guest_token_url, follow=True) + response = self.client.post(self.superset_guest_token_url) + mock_has_object_permission.assert_called_once() + self.assertEqual(response.status_code, 200) self.assertEqual(response.json().get("guestToken"), "test-token") mock_generate_guest_token.assert_called_once() diff --git a/platform_plugin_aspects/urls.py b/platform_plugin_aspects/urls.py index b142e4e..cddaa03 100644 --- a/platform_plugin_aspects/urls.py +++ b/platform_plugin_aspects/urls.py @@ -14,7 +14,7 @@ urlpatterns = [ re_path( rf"superset_guest_token/{COURSE_ID_PATTERN}/?$", - views.superset_guest_token, + views.SupersetView.as_view(), name="superset_guest_token", ), ] diff --git a/platform_plugin_aspects/utils.py b/platform_plugin_aspects/utils.py index 24277b2..f64436e 100644 --- a/platform_plugin_aspects/utils.py +++ b/platform_plugin_aspects/utils.py @@ -92,7 +92,9 @@ def generate_guest_token(user, course, dashboards, filters): Args: user: User object. - course: string Course ID + 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. diff --git a/platform_plugin_aspects/views.py b/platform_plugin_aspects/views.py index 38d739f..b0270ca 100644 --- a/platform_plugin_aspects/views.py +++ b/platform_plugin_aspects/views.py @@ -2,47 +2,119 @@ Endpoints for the Aspects platform plugin. """ +from collections import namedtuple + from django.conf import settings -from django.contrib.auth.decorators import login_required from django.core.exceptions import ImproperlyConfigured -from django.http import JsonResponse -from django.views.decorators.http import require_POST +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from rest_framework import exceptions, permissions +from rest_framework.authentication import SessionAuthentication +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 +except ImportError: + + class IsCourseStaffInstructor(permissions.BasePermission): + """ + Permission class to use during tests. -from .utils import _, generate_guest_token + 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 -@require_POST -@login_required -def superset_guest_token(request, course_id): + +# 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): """ - Return a guest token for Superset that the given request.user can use to access the Superset API. + Superset-related endpoints provided by the aspects platform plugin. """ - # TODO -- restrict access to course staff? how? - - course_key = CourseKey.from_string(course_id) - built_in_filters = [ - f"org = '{course_key.org}'", - # TODO? Need to fetch the Course object to add this filter. - # f"course_name = '{course_key.display_name}'", - f"course_run = '{course_key.run}'", - ] - dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS - extra_filters_format = settings.SUPERSET_EXTRA_FILTERS_FORMAT - - guest_token, exception = generate_guest_token( - user=request.user, - course=course_id, - dashboards=dashboards, - filters=built_in_filters + extra_filters_format, + + authentication_classes = (SessionAuthentication,) + permission_classes = ( + permissions.IsAuthenticated, + IsCourseStaffInstructor, ) - if not guest_token: - raise ImproperlyConfigured( - _( - "Unable to fetch Superset guest token, " - "mostly likely due to invalid settings.SUPERSET_CONFIG: {exception}" - ).format(exception=exception) + lookup_field = "course_id" + + @property + def allowed_methods(self): + """ + Only POST is allowed for this view. + """ + return ["post", "options", "head"] + + 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 exceptions.NotFound( + _("Invalid course id: '{course_id}'").format(course_id=course_id) + ) from exc + + # Fetch the CourseOverview (if we're running in edx-platform) + CourseOverview = get_model("course_overviews") + if CourseOverview: + course_overview = CourseOverview.objects.get(id=course_key) + course = Course( + course_id=course_key, display_name=course_overview.display_name + ) + else: + course = Course(course_id=course_key, display_name="") + + # May raise a permission denied + self.check_object_permissions(self.request, course) + + return course + + def post(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 + + guest_token, exception = generate_guest_token( + user=request.user, + course=course, + dashboards=dashboards, + filters=built_in_filters + extra_filters_format, ) - return JsonResponse({"guestToken": guest_token}) + if not guest_token: + raise ImproperlyConfigured( + _( + "Unable to fetch Superset guest token, " + "mostly likely due to invalid settings.SUPERSET_CONFIG: {exception}" + ).format(exception=exception) + ) + + return Response({"guestToken": guest_token}) diff --git a/requirements/test.in b/requirements/test.in index 9c69ca6..1bc4cb8 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -9,4 +9,3 @@ code-annotations # provides commands used by the pii_check make target. responses # mocks for the requests library ddt django-mock-queries -xblock-sdk # for testing XBlock handlers diff --git a/test_settings.py b/test_settings.py index 14a50ab..7e8ad0f 100644 --- a/test_settings.py +++ b/test_settings.py @@ -36,16 +36,11 @@ SECRET_KEY = 'very-insecure-key' MIDDLEWARE = [ - 'django.middleware.csrf.CsrfViewMiddleware', 'django.contrib.sessions.middleware.SessionMiddleware', 'django.contrib.auth.middleware.AuthenticationMiddleware', ] EVENT_SINK_CLICKHOUSE_MODEL_CONFIG = { - "auth_user": { - "module": "django.contrib.auth.models", - "model": "User", - }, "user_profile": { "module": "common.djangoapps.student.models", "model": "UserProfile", @@ -54,14 +49,6 @@ "module": "openedx.core.djangoapps.content.course_overviews.models", "model": "CourseOverview", }, - "external_id": { - "module": "openedx.core.djangoapps.external_user_ids.models", - "model": "ExternalId", - }, - "user_preference": { - "module": "openedx.core.djangoapps.user_api.models", - "model": "UserPreference", - }, } EVENT_SINK_CLICKHOUSE_COURSE_OVERVIEWS_ENABLED = True From bc89faecfc93297154fd987afd5b9c4ac1b23617 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Apr 2024 12:43:20 +0930 Subject: [PATCH 05/13] fix: quality --- platform_plugin_aspects/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/platform_plugin_aspects/views.py b/platform_plugin_aspects/views.py index b0270ca..b205328 100644 --- a/platform_plugin_aspects/views.py +++ b/platform_plugin_aspects/views.py @@ -15,7 +15,6 @@ from .utils import _, generate_guest_token, get_model - try: from openedx.core.lib.api.permissions import IsCourseStaffInstructor except ImportError: From 8686c0d369936d969e5b2102d72bd1519c34aa2b Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Apr 2024 13:58:30 +0930 Subject: [PATCH 06/13] refactor: refactors generate_guest_token to raise an exception instead of returning a tuple. Adds tests to improve test coverage --- platform_plugin_aspects/tests/test_utils.py | 18 ++--- platform_plugin_aspects/tests/test_views.py | 82 +++++++++++++++++++- platform_plugin_aspects/tests/test_xblock.py | 31 +++++++- platform_plugin_aspects/utils.py | 37 +++++---- platform_plugin_aspects/views.py | 42 +++++----- platform_plugin_aspects/xblock.py | 23 +++--- test_settings.py | 8 +- 7 files changed, 169 insertions(+), 72 deletions(-) diff --git a/platform_plugin_aspects/tests/test_utils.py b/platform_plugin_aspects/tests/test_utils.py index 0b7f95a..ccc187b 100644 --- a/platform_plugin_aspects/tests/test_utils.py +++ b/platform_plugin_aspects/tests/test_utils.py @@ -5,6 +5,7 @@ from unittest.mock import Mock, patch from django.conf import settings +from django.core.exceptions import ImproperlyConfigured from django.test import TestCase from platform_plugin_aspects.utils import ( @@ -144,16 +145,15 @@ def test_generate_guest_token_with_superset_client_exception( user_mock = Mock() mock_superset_client.side_effect = Exception("test-exception") - token, exception = generate_guest_token( - user=user_mock, - course=COURSE_ID, - dashboards=[{"name": "test", "uuid": "test-dashboard-uuid"}], - filters=[filter_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() - self.assertIsNone(token) - self.assertEqual(str(exception), "test-exception") @patch.object( settings, @@ -180,7 +180,7 @@ def test_generate_guest_token_succesful(self, mock_superset_client): user_mock = Mock() dashboards = [{"name": "test", "uuid": "test-dashboard-uuid"}] - token, _errors = generate_guest_token( + token = generate_guest_token( user=user_mock, course=COURSE_ID, dashboards=dashboards, diff --git a/platform_plugin_aspects/tests/test_views.py b/platform_plugin_aspects/tests/test_views.py index d0291e1..af004d5 100644 --- a/platform_plugin_aspects/tests/test_views.py +++ b/platform_plugin_aspects/tests/test_views.py @@ -2,14 +2,17 @@ Test views. """ -from unittest.mock import patch +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 django.urls import reverse +from opaque_keys.edx.keys import CourseKey from rest_framework.test import APIClient -from ..views import IsCourseStaffInstructor +from ..views import Course, IsCourseStaffInstructor COURSE_ID = "course-v1:org+course+run" User = get_user_model() @@ -46,16 +49,87 @@ def test_guest_token_requires_course_access(self): response = self.client.post(self.superset_guest_token_url) self.assertEqual(response.status_code, 403) + def test_guest_token_invalid_course_id(self): + superset_guest_token_url = reverse( + "superset_guest_token", + kwargs={"course_id": "block-v1:org+course+run"}, + ) + self.client.login(username="user", password="password") + response = self.client.post(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.post(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", "test-dashboard-uuid") + mock_generate_guest_token.return_value = "test-token" self.client.login(username="user", password="password") response = self.client.post(self.superset_guest_token_url) - mock_has_object_permission.assert_called_once() 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.post(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.post(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 552713f..354b52d 100644 --- a/platform_plugin_aspects/tests/test_xblock.py +++ b/platform_plugin_aspects/tests/test_xblock.py @@ -6,6 +6,7 @@ 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 @@ -121,16 +122,38 @@ def test_render_no_translations( assert resource.kind != "url" mock_get_language.assert_called_once() - @patch("platform_plugin_aspects.xblock.generate_guest_token") - def test_guest_token_handler(self, mock_generate_guest_token): - mock_generate_guest_token.return_value = ("test-token", "test-dashboard-uuid") + @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_generate_guest_token.assert_called_once() + 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/utils.py b/platform_plugin_aspects/utils.py index f64436e..d9c0ee0 100644 --- a/platform_plugin_aspects/utils.py +++ b/platform_plugin_aspects/utils.py @@ -10,6 +10,7 @@ from importlib import import_module from django.conf import settings +from django.core.exceptions import ImproperlyConfigured from django.urls import NoReverseMatch, reverse from supersetapiclient.client import SupersetClient from xblock.reference.user_service import XBlockUser @@ -31,7 +32,7 @@ def generate_superset_context( context, dashboards, language=None, -): +) -> dict: """ Update context with superset token and dashboard id. @@ -86,9 +87,11 @@ def generate_superset_context( return context -def generate_guest_token(user, course, 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. @@ -108,16 +111,6 @@ def generate_guest_token(user, course, 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 = { @@ -129,6 +122,12 @@ def generate_guest_token(user, course, dashboards, filters): } try: + client = SupersetClient( + host=superset_internal_host, + 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/", @@ -138,10 +137,16 @@ def generate_guest_token(user, course, dashboards, filters): response.raise_for_status() token = response.json()["token"] - return token, dashboards - except Exception as exc: # pylint: disable=broad-except + return token + + 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 index b205328..9ad6c4a 100644 --- a/platform_plugin_aspects/views.py +++ b/platform_plugin_aspects/views.py @@ -8,8 +8,9 @@ from django.core.exceptions import ImproperlyConfigured from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from rest_framework import exceptions, permissions +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 @@ -68,19 +69,23 @@ def get_object(self): try: course_key = CourseKey.from_string(course_id) except InvalidKeyError as exc: - raise exceptions.NotFound( + 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: - course_overview = CourseOverview.objects.get(id=course_key) - course = Course( - course_id=course_key, display_name=course_overview.display_name - ) - else: - course = Course(course_id=course_key, display_name="") + 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) @@ -101,19 +106,14 @@ def post(self, request, *args, **kwargs): dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS extra_filters_format = settings.SUPERSET_EXTRA_FILTERS_FORMAT - guest_token, exception = generate_guest_token( - user=request.user, - course=course, - dashboards=dashboards, - filters=built_in_filters + extra_filters_format, - ) - - if not guest_token: - raise ImproperlyConfigured( - _( - "Unable to fetch Superset guest token, " - "mostly likely due to invalid settings.SUPERSET_CONFIG: {exception}" - ).format(exception=exception) + 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 06eb462..aa6e146 100644 --- a/platform_plugin_aspects/xblock.py +++ b/platform_plugin_aspects/xblock.py @@ -6,6 +6,7 @@ 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 @@ -194,21 +195,15 @@ def get_superset_guest_token( user_service = self.runtime.service(self, "user") user = user_service.get_current_user() - guest_token, exception = generate_guest_token( - user=user, - course=self.runtime.course_id, - dashboards=self.dashboards(), - filters=self.filters, - ) - - if not guest_token: - raise JsonHandlerError( - 500, - _( - "Unable to fetch Superset guest token, " - "mostly likely due to invalid settings.SUPERSET_CONFIG: {exception}" - ).format(exception=exception), + 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}), diff --git a/test_settings.py b/test_settings.py index 7e8ad0f..41dd0ef 100644 --- a/test_settings.py +++ b/test_settings.py @@ -31,13 +31,13 @@ "platform_plugin_aspects", ) -ROOT_URLCONF = 'platform_plugin_aspects.urls' +ROOT_URLCONF = "platform_plugin_aspects.urls" -SECRET_KEY = 'very-insecure-key' +SECRET_KEY = "very-insecure-key" MIDDLEWARE = [ - 'django.contrib.sessions.middleware.SessionMiddleware', - 'django.contrib.auth.middleware.AuthenticationMiddleware', + "django.contrib.sessions.middleware.SessionMiddleware", + "django.contrib.auth.middleware.AuthenticationMiddleware", ] EVENT_SINK_CLICKHOUSE_MODEL_CONFIG = { From d804eb0f2937a74a89fe58710f14fe4f93914a77 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Apr 2024 15:10:05 +0930 Subject: [PATCH 07/13] fix: allow global staff OR course staff/instructors access to the superset token view --- platform_plugin_aspects/views.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/platform_plugin_aspects/views.py b/platform_plugin_aspects/views.py index 9ad6c4a..90fc407 100644 --- a/platform_plugin_aspects/views.py +++ b/platform_plugin_aspects/views.py @@ -17,7 +17,10 @@ from .utils import _, generate_guest_token, get_model try: - from openedx.core.lib.api.permissions import IsCourseStaffInstructor + from openedx.core.lib.api.permissions import ( + IsCourseStaffInstructor, + IsStaffOrReadOnly, + ) except ImportError: class IsCourseStaffInstructor(permissions.BasePermission): @@ -34,6 +37,20 @@ def has_object_permission(self, request, view, obj): """ 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 @@ -49,7 +66,7 @@ class SupersetView(GenericAPIView): authentication_classes = (SessionAuthentication,) permission_classes = ( permissions.IsAuthenticated, - IsCourseStaffInstructor, + IsStaffOrReadOnly | IsCourseStaffInstructor, ) lookup_field = "course_id" From 569f3952d7aad87c879778f8793af9ed812b2b01 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Apr 2024 15:11:42 +0930 Subject: [PATCH 08/13] fix: fixed namespace issue with urls so that reverse("platform_plugin_aspects:superset_guest_token") works whether we're in standalone tests or a plugin to the platform. --- platform_plugin_aspects/apps.py | 2 +- platform_plugin_aspects/tests/test_views.py | 11 ++------- platform_plugin_aspects/urls.py | 19 ++++++++++------ platform_plugin_aspects/utils.py | 25 +++++---------------- platform_plugin_aspects/xblock.py | 3 ++- 5 files changed, 22 insertions(+), 38 deletions(-) diff --git a/platform_plugin_aspects/apps.py b/platform_plugin_aspects/apps.py index 36da0ea..64197aa 100644 --- a/platform_plugin_aspects/apps.py +++ b/platform_plugin_aspects/apps.py @@ -16,7 +16,7 @@ class PlatformPluginAspectsConfig(AppConfig): plugin_app = { PluginURLs.CONFIG: { "lms.djangoapp": { - PluginURLs.NAMESPACE: name, + PluginURLs.NAMESPACE: "", PluginURLs.REGEX: r"^aspects/", PluginURLs.RELATIVE_PATH: "urls", }, diff --git a/platform_plugin_aspects/tests/test_views.py b/platform_plugin_aspects/tests/test_views.py index af004d5..ae19798 100644 --- a/platform_plugin_aspects/tests/test_views.py +++ b/platform_plugin_aspects/tests/test_views.py @@ -8,7 +8,6 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ImproperlyConfigured, ObjectDoesNotExist from django.test import TestCase -from django.urls import reverse from opaque_keys.edx.keys import CourseKey from rest_framework.test import APIClient @@ -29,10 +28,7 @@ def setUp(self): """ super().setUp() self.client = APIClient() - self.superset_guest_token_url = reverse( - "superset_guest_token", - kwargs={"course_id": COURSE_ID}, - ) + self.superset_guest_token_url = f"/superset_guest_token/{COURSE_ID}" self.user = User.objects.create( username="user", email="user@example.com", @@ -50,10 +46,7 @@ def test_guest_token_requires_course_access(self): self.assertEqual(response.status_code, 403) def test_guest_token_invalid_course_id(self): - superset_guest_token_url = reverse( - "superset_guest_token", - kwargs={"course_id": "block-v1:org+course+run"}, - ) + superset_guest_token_url = "/superset_guest_token/block-v1:org+course+run" self.client.login(username="user", password="password") response = self.client.post(superset_guest_token_url) self.assertEqual(response.status_code, 404) diff --git a/platform_plugin_aspects/urls.py b/platform_plugin_aspects/urls.py index cddaa03..fd8b8d2 100644 --- a/platform_plugin_aspects/urls.py +++ b/platform_plugin_aspects/urls.py @@ -2,19 +2,24 @@ Urls for the Aspects plugin. """ -from django.urls import re_path +from django.urls import include, path, re_path from . import views # Copied from openedx.core.constants COURSE_ID_PATTERN = r"(?P[^/+]+(/|\+)[^/+]+(/|\+)[^/?]+)" -app_name = "platform_plugin_aspects" +app_url_patterns = ( + [ + re_path( + rf"superset_guest_token/{COURSE_ID_PATTERN}/?$", + views.SupersetView.as_view(), + name="superset_guest_token", + ), + ], + "platform_plugin_aspects", +) urlpatterns = [ - re_path( - rf"superset_guest_token/{COURSE_ID_PATTERN}/?$", - views.SupersetView.as_view(), - name="superset_guest_token", - ), + path("", include(app_url_patterns)), ] diff --git a/platform_plugin_aspects/utils.py b/platform_plugin_aspects/utils.py index d9c0ee0..40e9c8b 100644 --- a/platform_plugin_aspects/utils.py +++ b/platform_plugin_aspects/utils.py @@ -11,7 +11,7 @@ from django.conf import settings from django.core.exceptions import ImproperlyConfigured -from django.urls import NoReverseMatch, reverse +from django.urls import reverse from supersetapiclient.client import SupersetClient from xblock.reference.user_service import XBlockUser @@ -56,25 +56,10 @@ def generate_superset_context( superset_url = _fix_service_url(superset_config.get("service_url")) - # FIXME -- namespace issue with plugin-registered urls? - try: - guest_token_url = reverse( - "platform_plugin_aspects:superset_guest_token", - kwargs={"course_id": course}, - ) - except NoReverseMatch: - logger.error( - "Error reversing platform_plugin_aspects:superset_guest_token, trying without namespace" - ) - try: - guest_token_url = reverse( - "superset_guest_token", - kwargs={"course_id": course}, - ) - logger.info("Reversing superset_guest_token worked") - except NoReverseMatch: - logger.critical("Error reversing superset_guest_token, giving up") - guest_token_url = "" + guest_token_url = reverse( + "platform_plugin_aspects:superset_guest_token", + kwargs={"course_id": course}, + ) context.update( { diff --git a/platform_plugin_aspects/xblock.py b/platform_plugin_aspects/xblock.py index aa6e146..33bd0b0 100644 --- a/platform_plugin_aspects/xblock.py +++ b/platform_plugin_aspects/xblock.py @@ -10,7 +10,8 @@ from django.utils import translation from web_fragments.fragment import Fragment from webob import Response -from xblock.core import JsonHandlerError, XBlock +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 4da80dd81b97c0d34a65e6c55bc6b5437d240f97 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Apr 2024 17:16:36 +0930 Subject: [PATCH 09/13] 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", From 89fd958277342275f3f0fb3a55acd03abfc85628 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 12 Apr 2024 11:41:34 -0500 Subject: [PATCH 10/13] fix: move fetchGuestToken out of embedDashboard function --- .../static/js/embed_dashboard.js | 62 ++++++++++--------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/platform_plugin_aspects/static/js/embed_dashboard.js b/platform_plugin_aspects/static/js/embed_dashboard.js index a0f61fb..1eba106 100644 --- a/platform_plugin_aspects/static/js/embed_dashboard.js +++ b/platform_plugin_aspects/static/js/embed_dashboard.js @@ -1,38 +1,40 @@ -function embedDashboard(dashboard_uuid, superset_url, guest_token_url, xblock_id) { - xblock_id = 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; - } +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; } + return cookieValue; +} - async function fetchGuestToken() { - return fetch(guest_token_url, { - method: 'POST', - headers: { - "X-CSRFToken": getCookie('csrftoken'), - }, - }).then(response => { - if (response.ok) { - const data = response.json(); - return data.guestToken; - } else { - console.error(response); - } - }); +async function fetchGuestToken() { + const response = await fetch(superset_guest_token_url, { + method: 'POST', + 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 @@ -61,6 +63,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, dashboard.uuid); + embedDashboard(dashboard.uuid, window.superset_url, dashboard.uuid); }); } From ba865c60bbfec06bdd480c21a51bd32aca9208a6 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 12 Apr 2024 12:22:12 -0500 Subject: [PATCH 11/13] fix: add cache for superset guest token endpoint --- platform_plugin_aspects/static/js/embed_dashboard.js | 2 +- platform_plugin_aspects/views.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/platform_plugin_aspects/static/js/embed_dashboard.js b/platform_plugin_aspects/static/js/embed_dashboard.js index 1eba106..fda579a 100644 --- a/platform_plugin_aspects/static/js/embed_dashboard.js +++ b/platform_plugin_aspects/static/js/embed_dashboard.js @@ -16,7 +16,7 @@ function getCookie(name) { async function fetchGuestToken() { const response = await fetch(superset_guest_token_url, { - method: 'POST', + method: 'GET', headers: { "X-CSRFToken": getCookie("csrftoken"), } diff --git a/platform_plugin_aspects/views.py b/platform_plugin_aspects/views.py index 90fc407..21ff3ba 100644 --- a/platform_plugin_aspects/views.py +++ b/platform_plugin_aspects/views.py @@ -6,6 +6,8 @@ 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 @@ -76,7 +78,7 @@ def allowed_methods(self): """ Only POST is allowed for this view. """ - return ["post", "options", "head"] + return ["get", "options", "head"] def get_object(self): """ @@ -109,7 +111,8 @@ def get_object(self): return course - def post(self, request, *args, **kwargs): + @method_decorator(cache_page(60 * 5)) + def get(self, request, *args, **kwargs): """ Return a guest token for accessing the Superset API. """ From 887168a60e337171dd29d4a52296950d900ddbed Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 12 Apr 2024 12:23:55 -0500 Subject: [PATCH 12/13] chore: bump version to v0.7.0 --- CHANGELOG.rst | 8 ++++++++ platform_plugin_aspects/__init__.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) 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__))) From a77a7f51f514875cf61c8588171e477e12f111aa Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 16 Apr 2024 15:31:12 +0930 Subject: [PATCH 13/13] test: fixes tests broken by change from POST to GET and removes unneeded code --- platform_plugin_aspects/tests/test_views.py | 14 +++++++------- platform_plugin_aspects/views.py | 7 ------- test_settings.py | 7 +++++++ 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/platform_plugin_aspects/tests/test_views.py b/platform_plugin_aspects/tests/test_views.py index ae19798..4d5a61e 100644 --- a/platform_plugin_aspects/tests/test_views.py +++ b/platform_plugin_aspects/tests/test_views.py @@ -37,18 +37,18 @@ def setUp(self): self.user.save() def test_guest_token_requires_authorization(self): - response = self.client.post(self.superset_guest_token_url) + 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.post(self.superset_guest_token_url) + 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.post(superset_guest_token_url) + response = self.client.get(superset_guest_token_url) self.assertEqual(response.status_code, 404) @patch("platform_plugin_aspects.views.get_model") @@ -60,7 +60,7 @@ def test_guest_token_course_not_found(self, mock_get_model): ) self.client.login(username="user", password="password") - response = self.client.post(self.superset_guest_token_url) + response = self.client.get(self.superset_guest_token_url) self.assertEqual(response.status_code, 404) mock_model_get.assert_called_once() @@ -71,7 +71,7 @@ def test_guest_token(self, mock_generate_guest_token, mock_has_object_permission mock_generate_guest_token.return_value = "test-token" self.client.login(username="user", password="password") - response = self.client.post(self.superset_guest_token_url) + response = self.client.get(self.superset_guest_token_url) self.assertEqual(response.status_code, 200) self.assertEqual(response.json().get("guestToken"), "test-token") @@ -87,7 +87,7 @@ def test_no_guest_token( mock_generate_guest_token.side_effect = ImproperlyConfigured self.client.login(username="user", password="password") - response = self.client.post(self.superset_guest_token_url) + response = self.client.get(self.superset_guest_token_url) self.assertEqual(response.status_code, 500) mock_has_object_permission.assert_called_once() @@ -108,7 +108,7 @@ def test_guest_token_with_course_overview( ) self.client.login(username="user", password="password") - response = self.client.post(self.superset_guest_token_url) + response = self.client.get(self.superset_guest_token_url) self.assertEqual(response.status_code, 200) self.assertEqual(response.json().get("guestToken"), "test-token") diff --git a/platform_plugin_aspects/views.py b/platform_plugin_aspects/views.py index 21ff3ba..7c4b0cf 100644 --- a/platform_plugin_aspects/views.py +++ b/platform_plugin_aspects/views.py @@ -73,13 +73,6 @@ class SupersetView(GenericAPIView): lookup_field = "course_id" - @property - def allowed_methods(self): - """ - Only POST is allowed for this view. - """ - return ["get", "options", "head"] - def get_object(self): """ Return a Course-like object for the requested course_id. diff --git a/test_settings.py b/test_settings.py index 174c410..8ab4b80 100644 --- a/test_settings.py +++ b/test_settings.py @@ -31,6 +31,13 @@ "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"