From e19c4eee8a7fc0b2e038191e8491a79eeeb3d334 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 10 Dec 2019 19:32:55 -0500 Subject: [PATCH] use LoginSessionView.post for logistration - retires toggle ENABLE_LOGIN_POST_WITHOUT_SHIM - permanently points to LoginSessionView.post which no longer has shim This is Part 2 of clean-up, and should be done once the toggle is no longer required and the shim is no longer required. ARCH-1253 --- .../core/djangoapps/user_authn/views/login.py | 178 +++--------------- .../djangoapps/user_authn/views/login_form.py | 21 +-- .../user_authn/views/tests/test_login.py | 97 +--------- 3 files changed, 39 insertions(+), 257 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 32a96e6c5c..6900171e72 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -336,6 +336,33 @@ def finish_auth(request): # pylint: disable=unused-argument def login_user(request): """ AJAX request to log in the user. + + Arguments: + request (HttpRequest) + + Required params: + email, password + + Optional params: + analytics: a JSON-encoded object with additional info to include in the login analytics event. The only + supported field is "enroll_course_id" to indicate that the user logged in while enrolling in a particular + course. + + Returns: + HttpResponse: 200 if successful. + Ex. {'success': true} + HttpResponse: 400 if the request failed. + Ex. {'success': false, 'value': '{'success': false, 'value: 'Email or password is incorrect.'} + HttpResponse: 403 if successful authentication with a third party provider but does not have a linked account. + Ex. {'success': false, 'error_code': 'third-party-auth-with-no-linked-account'} + + Example Usage: + + POST /login_ajax + with POST params `email`, `password` + + 200 {'success': true} + """ _parse_analytics_param_for_course_id(request) @@ -444,34 +471,17 @@ class LoginSessionView(APIView): def post(self, request): """Log in a user. - You must send all required form fields with the request. - - You can optionally send an `analytics` param with a JSON-encoded - object with additional info to include in the login analytics event. - Currently, the only supported field is "enroll_course_id" to indicate - that the user logged in while enrolling in a particular course. - - Arguments: - request (HttpRequest) - - Returns: - HttpResponse: 200 on success - HttpResponse: 400 if the request is not valid. - HttpResponse: 403 if authentication failed. - 403 with content "third-party-auth" if the user - has successfully authenticated with a third party provider - but does not have a linked account. - HttpResponse: 302 if redirecting to another page. + See `login_user` for details. Example Usage: POST /user_api/v1/login_session - with POST params `email`, `password`, and `remember`. + with POST params `email`, `password`. - 200 OK + 200 {'success': true} """ - return shim_student_view(login_user, check_logged_in=True)(request) + return login_user(request) @method_decorator(sensitive_post_parameters("password")) def dispatch(self, request, *args, **kwargs): @@ -507,129 +517,3 @@ def _parse_analytics_param_for_course_id(request): analytics=analytics ) ) - - -def shim_student_view(view_func, check_logged_in=False): - """Create a "shim" view for a view function from the student Django app. - - UPDATE: This shim is only used to wrap `login_user`, which now lives in - the user_authn Django app (not the student app). - - Specifically, we need to: - * Strip out enrollment params, since the client for the new registration/login - page will communicate with the enrollment API to update enrollments. - - * Return responses with HTTP status codes indicating success/failure - (instead of always using status 200, but setting "success" to False in - the JSON-serialized content of the response) - - * Use status code 403 to indicate a login failure. - - The shim will preserve any cookies set by the view. - - Arguments: - view_func (function): The view function from the student Django app. - - Keyword Args: - check_logged_in (boolean): If true, check whether the user successfully - authenticated and if not set the status to 403. This argument is - used to test the shim by skipping this check. - - Returns: - function - - """ - @wraps(view_func) - def _inner(request): # pylint: disable=missing-docstring - # Call the original view to generate a response. - # We can safely modify the status code or content - # of the response, but to be safe we won't mess - # with the headers. - response = view_func(request) - - # Most responses from this view are JSON-encoded - # dictionaries with keys "success", "value", and - # (sometimes) "redirect_url". - # - # We want to communicate some of this information - # using HTTP status codes instead. - # - # We ignore the "redirect_url" parameter, because we don't need it: - # 1) It's used to redirect on change enrollment, which - # our client will handle directly - # (that's why we strip out the enrollment params from the request) - # 2) It's used by third party auth when a user has already successfully - # authenticated and we're not sending login credentials. However, - # this case is never encountered in practice: on the old login page, - # the login form would be submitted directly, so third party auth - # would always be "trumped" by first party auth. If a user has - # successfully authenticated with us, we redirect them to the dashboard - # regardless of how they authenticated; and if a user is completing - # the third party auth pipeline, we redirect them from the pipeline - # completion end-point directly. - try: - response_dict = json.loads(response.content.decode('utf-8')) - msg = response_dict.get("value", u"") - success = response_dict.get("success") - set_custom_metric('shim_original_response_is_json', True) - set_custom_metric('shim_original_redirect_url', response_dict.get("redirect_url")) - set_custom_metric('shim_original_redirect', response_dict.get("redirect")) - except (ValueError, TypeError): - msg = response.content - success = True - set_custom_metric('shim_original_response_is_json', False) - set_custom_metric('shim_original_response_msg', msg) - set_custom_metric('shim_original_response_success', success) - set_custom_metric('shim_original_response_status', response.status_code) - - # If the user is not authenticated when we expect them to be - # send the appropriate status code. - # We check whether the user attribute is set to make - # it easier to test this without necessarily running - # the request through authentication middleware. - is_authenticated = ( - getattr(request, 'user', None) is not None - and request.user.is_authenticated - ) - if check_logged_in and not is_authenticated: - # If we get a 403 status code from the student view - # this means we've successfully authenticated with a - # third party provider, but we don't have a linked - # EdX account. Send a helpful error code so the client - # knows this occurred. - if response.status_code == 403: - response.content = "third-party-auth" - - # Otherwise, it's a general authentication failure. - # Ensure that the status code is a 403 and pass - # along the message from the view. - else: - response.status_code = 403 - response.content = msg - - # If an error condition occurs, send a status 400 - elif response.status_code != 200 or not success: - # login_user sends status 200 even when an error occurs - # If the JSON-serialized content has a value "success" set to False, - # then we know an error occurred. - # NOTE: temporary metric added so we can remove this code once the - # original response is 400 instead of 200. - set_custom_metric('shim_adjusted_status_code', bool(response.status_code == 200)) - if response.status_code == 200: - response.status_code = 400 - response.content = msg - - # If the response is successful, then return the content - # of the response directly rather than including it - # in a JSON-serialized dictionary. - else: - response.content = msg - - set_custom_metric('shim_final_response_msg', response.content) - set_custom_metric('shim_final_response_status', response.status_code) - # Return the response, preserving the original headers. - # This is really important, since the student views set cookies - # that are used elsewhere in the system (such as the marketing site). - return response - - return _inner diff --git a/openedx/core/djangoapps/user_authn/views/login_form.py b/openedx/core/djangoapps/user_authn/views/login_form.py index faeab3ef95..6397103850 100644 --- a/openedx/core/djangoapps/user_authn/views/login_form.py +++ b/openedx/core/djangoapps/user_authn/views/login_form.py @@ -77,20 +77,6 @@ def _apply_third_party_auth_overrides(request, form_desc): ) -# .. toggle_name: FEATURES[ENABLE_LOGIN_POST_WITHOUT_SHIM] -# .. toggle_implementation: DjangoSetting -# .. toggle_default: False -# .. toggle_description: Toggle for enabling login post without shim_student_view (using `login_api`). -# .. toggle_category: n/a -# .. toggle_use_cases: incremental_release -# .. toggle_creation_date: 2019-12-10 -# .. toggle_expiration_date: 2020-06-01 -# .. toggle_warnings: n/a -# .. toggle_tickets: ARCH-1253 -# .. toggle_status: supported -ENABLE_LOGIN_POST_WITHOUT_SHIM = 'ENABLE_LOGIN_POST_WITHOUT_SHIM' - - def get_login_session_form(request): """Return a description of the login form. @@ -105,12 +91,7 @@ def get_login_session_form(request): HttpResponse """ - if settings.FEATURES.get(ENABLE_LOGIN_POST_WITHOUT_SHIM): - submit_url = reverse("login_api") - else: - submit_url = reverse("user_api_login_session") - - form_desc = FormDescription("post", submit_url) + form_desc = FormDescription("post", reverse("user_api_login_session")) _apply_third_party_auth_overrides(request, form_desc) # Translators: This label appears above a field on the login form diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_login.py b/openedx/core/djangoapps/user_authn/views/tests/test_login.py index df33328efa..c63b44edd3 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -30,17 +30,14 @@ from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRI from openedx.core.djangoapps.user_api.accounts import EMAIL_MIN_LENGTH, EMAIL_MAX_LENGTH from openedx.core.djangoapps.user_authn.cookies import jwt_cookies from openedx.core.djangoapps.user_authn.views.login import ( - shim_student_view, AllowedAuthUser, ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY ) -from openedx.core.djangoapps.user_authn.views.login_form import ENABLE_LOGIN_POST_WITHOUT_SHIM from openedx.core.djangoapps.user_authn.tests.utils import setup_login_oauth_client from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin from openedx.core.lib.api.test_utils import ApiTestCase from student.tests.factories import RegistrationFactory, UserFactory, UserProfileFactory -from util.json_request import JsonResponse from util.password_policy_validators import DEFAULT_MAX_PASSWORD_LENGTH @@ -662,26 +659,15 @@ class LoginSessionViewTest(ApiTestCase): response = self.client.patch(self.url) self.assertHttpMethodNotAllowed(response) - @ddt.data( - {ENABLE_LOGIN_POST_WITHOUT_SHIM: True}, - {ENABLE_LOGIN_POST_WITHOUT_SHIM: False}, - {}, - ) - def test_login_form(self, features_setting): - with patch.dict("django.conf.settings.FEATURES", features_setting): - # Retrieve the login form - response = self.client.get(self.url, content_type="application/json") - self.assertHttpOK(response) - - if ENABLE_LOGIN_POST_WITHOUT_SHIM in features_setting and features_setting[ENABLE_LOGIN_POST_WITHOUT_SHIM]: - submit_url = reverse("login_api") - else: - submit_url = reverse("user_api_login_session") + def test_login_form(self): + # Retrieve the login form + response = self.client.get(self.url, content_type="application/json") + self.assertHttpOK(response) # Verify that the form description matches what we expect form_desc = json.loads(response.content.decode('utf-8')) self.assertEqual(form_desc["method"], "post") - self.assertEqual(form_desc["submit_url"], submit_url) + self.assertEqual(form_desc["submit_url"], reverse("user_api_login_session")) self.assertEqual(form_desc["fields"], [ { "name": "email", @@ -785,14 +771,14 @@ class LoginSessionViewTest(ApiTestCase): "email": self.EMAIL, "password": "invalid" }) - self.assertHttpForbidden(response) + self.assertHttpBadRequest(response) # Invalid email address response = self.client.post(self.url, { "email": "invalid@example.com", "password": self.PASSWORD, }) - self.assertHttpForbidden(response) + self.assertHttpBadRequest(response) def test_missing_login_params(self): # Create a test user @@ -812,72 +798,3 @@ class LoginSessionViewTest(ApiTestCase): # Missing both email and password response = self.client.post(self.url, {}) - - -@ddt.ddt -class StudentViewShimTest(TestCase): - "Tests of the student view shim." - def setUp(self): - super(StudentViewShimTest, self).setUp() - self.captured_request = None - - def test_third_party_auth_login_failure(self): - mocked_response_content = { - "success": False, - "error_code": "third-party-auth-with-no-linked-account", - "value": "Test message." - } - view = self._shimmed_view( - JsonResponse(mocked_response_content, status=403), - check_logged_in=True - ) - response = view(HttpRequest()) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.content, b"third-party-auth") - - def test_non_json_response(self): - view = self._shimmed_view(HttpResponse(content="Not a JSON dict")) - response = view(HttpRequest()) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.content, b"Not a JSON dict") - - @ddt.data("redirect", "redirect_url") - def test_ignore_redirect_from_json(self, redirect_key): - view = self._shimmed_view( - HttpResponse(content=json.dumps({ - "success": True, - redirect_key: "/redirect" - })) - ) - response = view(HttpRequest()) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.content.decode('utf-8'), "") - - def test_error_from_json(self): - view = self._shimmed_view( - HttpResponse(content=json.dumps({ - "success": False, - "value": "Error!" - })) - ) - response = view(HttpRequest()) - self.assertEqual(response.status_code, 400) - self.assertEqual(response.content, b"Error!") - - def test_preserve_headers(self): - view_response = HttpResponse() - view_response["test-header"] = "test" - view = self._shimmed_view(view_response) - response = view(HttpRequest()) - self.assertEqual(response["test-header"], "test") - - def test_check_logged_in(self): - view = self._shimmed_view(HttpResponse(), check_logged_in=True) - response = view(HttpRequest()) - self.assertEqual(response.status_code, 403) - - def _shimmed_view(self, response, check_logged_in=False): - def stub_view(request): - self.captured_request = request - return response - return shim_student_view(stub_view, check_logged_in=check_logged_in)