diff --git a/openedx/core/djangoapps/user_authn/views/login_form.py b/openedx/core/djangoapps/user_authn/views/login_form.py index 3729c40868..bb78a9df1a 100644 --- a/openedx/core/djangoapps/user_authn/views/login_form.py +++ b/openedx/core/djangoapps/user_authn/views/login_form.py @@ -187,7 +187,12 @@ def login_and_registration_form(request, initial_mode="login"): log.exception("Unknown tpa_hint provider: %s", ex) # Redirect to authn MFE if it is enabled - # except if user is an enterprise user with tpa_hint_provider coming from a SAML IDP. + # AND + # user is not an enterprise user + # AND + # tpa_hint_provider is not available + # AND + # user is not coming from a SAML IDP. saml_provider = False running_pipeline = pipeline.get(request) if running_pipeline: @@ -197,8 +202,10 @@ def login_and_registration_form(request, initial_mode="login"): enterprise_customer = enterprise_customer_for_request(request) - if should_redirect_to_authn_microfrontend() and not \ - (enterprise_customer and tpa_hint_provider and saml_provider): + if should_redirect_to_authn_microfrontend() and \ + not enterprise_customer and \ + not tpa_hint_provider and \ + not saml_provider: # This is to handle a case where a logged-in cookie is not present but the user is authenticated. # Note: If we don't handle this learner is redirected to authn MFE and then back to dashboard diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py index c187e7e6c0..3220cd5139 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py @@ -648,75 +648,6 @@ class LoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleSto assert response['Content-Language'] == 'es-es' - @ddt.data( - (None, None, None, True), - ({'name': 'Test Enterprise', 'uuid': 'test-uuid'}, None, None, True), - ({'name': 'Test Enterprise', 'uuid': 'test-uuid'}, 'test-provider', None, True), - ({'name': 'Test Enterprise', 'uuid': 'test-uuid'}, 'test-provider', True, False) - ) - @ddt.unpack - @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) - def test_enterprise_saml_redirection(self, enterprise_customer_data, provider_id, is_saml, should_redirect): - """ - Test that authentication MFE redirection respects the enterprise + SAML provider conditions. - In particular, verify that if we have an enterprise customer with a SAML-based tpa_hint_provider, - we do NOT redirect to the MFE, but handle the request in LMS. All other combinations should - redirect to the MFE when it's enabled. - """ - if provider_id and is_saml: - self.enable_saml() - self._configure_testshib_provider('TestShib', provider_id) - - with ( - mock.patch( - 'openedx.core.djangoapps.user_authn.views.login_form.enterprise_customer_for_request' - ) as mock_ec, - mock.patch( - 'openedx.core.djangoapps.user_authn.views.login_form.should_redirect_to_authn_microfrontend' - ) as mock_should_redirect, - mock.patch( - 'openedx.core.djangoapps.user_authn.views.login_form.third_party_auth.utils.is_saml_provider' - ) as mock_is_saml - ): - mock_ec.return_value = enterprise_customer_data - mock_should_redirect.return_value = should_redirect - mock_is_saml.return_value = (True, None) if is_saml else (False, None) - - params = {} - if provider_id: - params['tpa_hint'] = provider_id - - if provider_id and is_saml: - pipeline_target = 'openedx.core.djangoapps.user_authn.views.login_form.third_party_auth.pipeline' - with mock.patch(pipeline_target + '.get') as mock_pipeline: - pipeline_data = { - 'backend': 'tpa-saml', - 'kwargs': { - 'response': { - 'idp_name': provider_id - }, - 'details': { - 'email': 'test@example.com', - 'fullname': 'Test User', - 'username': 'testuser' - } - } - } - mock_pipeline.return_value = pipeline_data - response = self.client.get(reverse('signin_user'), params) - else: - response = self.client.get(reverse('signin_user'), params) - - if should_redirect: - self.assertRedirects( - response, - settings.AUTHN_MICROFRONTEND_URL + '/login' + - ('?' + urlencode(params) if params else ''), - fetch_redirect_response=False - ) - else: - self.assertEqual(response.status_code, 200) - @skip_unless_lms class AccountCreationTestCaseWithSiteOverrides(SiteMixin, TestCase): diff --git a/xmodule/capa/safe_exec/remote_exec.py b/xmodule/capa/safe_exec/remote_exec.py index 5b231eb156..fb086b1064 100644 --- a/xmodule/capa/safe_exec/remote_exec.py +++ b/xmodule/capa/safe_exec/remote_exec.py @@ -7,7 +7,7 @@ import logging from importlib import import_module import requests -from codejail.safe_exec import SafeExecException +from codejail.safe_exec import SafeExecException, json_safe from django.conf import settings from edx_toggles.toggles import SettingToggle from requests.exceptions import RequestException, HTTPError @@ -90,7 +90,21 @@ def send_safe_exec_request_v0(data): extra_files = data.pop("extra_files") codejail_service_endpoint = get_codejail_rest_service_endpoint() - payload = json.dumps(data) + + # In rare cases an XBlock might introduce `bytes` objects (or other + # non-JSON-serializable objects) into the globals dict. The codejail service + # (via the codejail library) will call `json_safe` on the globals before + # JSON-encoding for the sandbox input, but here we need to call it earlier + # in the process so we can even transport the globals *to* the codejail + # service. Otherwise, we may get a TypeError when constructing the payload. + # + # This is a lossy operation (non-serializable objects will be dropped, and + # bytes converted to strings) but it is the same lossy operation that + # codejail will perform anyhow -- and it should be idempotent. + data_send = {**data} + data_send['globals_dict'] = json_safe(data_send['globals_dict']) + + payload = json.dumps(data_send) try: response = requests.post( diff --git a/xmodule/capa/safe_exec/tests/test_remote_exec.py b/xmodule/capa/safe_exec/tests/test_remote_exec.py new file mode 100644 index 0000000000..ee1ee49383 --- /dev/null +++ b/xmodule/capa/safe_exec/tests/test_remote_exec.py @@ -0,0 +1,32 @@ +""" +Tests for remote codejail execution. +""" + +import json +from unittest import TestCase +from unittest.mock import patch + +from django.test import override_settings + +from xmodule.capa.safe_exec.remote_exec import get_remote_exec + + +class TestRemoteExec(TestCase): + """Tests for remote_exec.""" + + @override_settings( + ENABLE_CODEJAIL_REST_SERVICE=True, + CODE_JAIL_REST_SERVICE_HOST='http://localhost', + ) + @patch('requests.post') + def test_json_encode(self, mock_post): + get_remote_exec({ + 'code': "out = 1 + 1", + 'globals_dict': {'some_data': b'bytes', 'unusable': object()}, + 'extra_files': None, + }) + + mock_post.assert_called_once() + data_arg = mock_post.call_args_list[0][1]['data'] + payload = json.loads(data_arg['payload']) + assert payload['globals_dict'] == {'some_data': 'bytes'}