From 5aa6181f852d7f88325283410c5c410fcddb601b Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Thu, 21 Nov 2019 16:20:30 -0500 Subject: [PATCH] switch login_user errors to 400 The APIs using login_user are currently not following the API conventions for non-SSO related authentication errors, by returning a 200 status code for errors. In addition to switching the status code from 200 => 400 for authentication failures, the following minor changes were made: - Document and refactor an existing authn switch. - Remove an unused url definition for login_ajax + error. BREAKING CHANGE: This changes /login_post and /login_ajax to return 400, rather than 200, when success=False in the returned JSON (for non-SSO related authentication errors). To remove risk around this change, it was added behind a waffle switch named `user_authn.update_login_user_error_status_code`. A breaking change was made, rather than introducing /login_ajax_new, in order to more quickly get to our end goal of the current clean-up effort of having a single function for login. If this breaks any callers, we may fix or abandon this change altogether. ARCH-1253 --- .../djangoapps/user_authn/config/waffle.py | 36 +++++++++++++--- .../core/djangoapps/user_authn/urls_common.py | 1 - .../core/djangoapps/user_authn/views/login.py | 15 +++++-- .../user_authn/views/tests/test_login.py | 41 ++++++++++++------- 4 files changed, 68 insertions(+), 25 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/config/waffle.py b/openedx/core/djangoapps/user_authn/config/waffle.py index 2589267d22..75cb4a801e 100644 --- a/openedx/core/djangoapps/user_authn/config/waffle.py +++ b/openedx/core/djangoapps/user_authn/config/waffle.py @@ -3,11 +3,37 @@ Waffle flags and switches for user authn. """ from __future__ import absolute_import -from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace +from openedx.core.djangoapps.waffle_utils import WaffleSwitch, WaffleSwitchNamespace -WAFFLE_NAMESPACE = u'user_authn' +_WAFFLE_NAMESPACE = u'user_authn' +_WAFFLE_SWITCH_NAMESPACE = WaffleSwitchNamespace(name=_WAFFLE_NAMESPACE, log_prefix=u'UserAuthN: ') -# If this switch is enabled then users must be sign in using their allowed domain SSO account -ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY = 'enable_login_using_thirdparty_auth_only' +# .. toggle_name: user_authn.enable_login_using_thirdparty_auth_only +# .. toggle_implementation: WaffleSwitch +# .. toggle_default: False +# .. toggle_description: When enabled, users must be sign in using their allowed domain SSO account. +# .. toggle_category: authn +# .. toggle_use_cases: incremental_release +# .. toggle_creation_date: 2019-11-20 +# .. toggle_expiration_date: 2020-01-31 +# .. toggle_warnings: Requires THIRD_PARTY_AUTH_ONLY_DOMAIN to also be set. +# .. toggle_tickets: ENT-2461 +# .. toggle_status: supported +ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY = WaffleSwitch( + _WAFFLE_SWITCH_NAMESPACE, 'enable_login_using_thirdparty_auth_only' +) -waffle = WaffleSwitchNamespace(name=WAFFLE_NAMESPACE, log_prefix=u'UserAuthN: ') +# .. toggle_name: user_authn.update_login_user_error_status_code +# .. toggle_implementation: WaffleSwitch +# .. toggle_default: False +# .. toggle_description: Changes auth failures (non-SSO) from 200 to 400. +# .. toggle_category: authn +# .. toggle_use_cases: incremental_release +# .. toggle_creation_date: 2019-11-21 +# .. toggle_expiration_date: 2020-01-31 +# .. toggle_warnings: Causes backward incompatible change. Document before removing. +# .. toggle_tickets: ARCH-1253 +# .. toggle_status: supported +UPDATE_LOGIN_USER_ERROR_STATUS_CODE = WaffleSwitch( + _WAFFLE_SWITCH_NAMESPACE, 'update_login_user_error_status_code' +) diff --git a/openedx/core/djangoapps/user_authn/urls_common.py b/openedx/core/djangoapps/user_authn/urls_common.py index 13aaf24991..8b3a5a5c1c 100644 --- a/openedx/core/djangoapps/user_authn/urls_common.py +++ b/openedx/core/djangoapps/user_authn/urls_common.py @@ -37,7 +37,6 @@ urlpatterns = [ # Login url(r'^login_post$', login.login_user, name='login_post'), url(r'^login_ajax$', login.login_user, name="login"), - url(r'^login_ajax/(?P[^/]*)$', login.login_user), # Moved from user_api/legacy_urls.py # `user_api` prefix is preserved for backwards compatibility. diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 81d7647a34..66602acc96 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -36,7 +36,7 @@ from openedx.core.djangoapps.util.user_messages import PageLevelMessages from openedx.core.djangoapps.user_authn.views.password_reset import send_password_reset_email_for_user from openedx.core.djangoapps.user_authn.config.waffle import ( ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY, - waffle as authn_waffle + UPDATE_LOGIN_USER_ERROR_STATUS_CODE ) from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.api.view_utils import require_post_params @@ -284,7 +284,7 @@ def _check_user_auth_flow(site, user): Check if user belongs to an allowed domain and not whitelisted then ask user to login through allowed domain SSO provider. """ - if user and authn_waffle.is_enabled(ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY): + if user and ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY.is_enabled(): allowed_domain = site.configuration.get_value('THIRD_PARTY_AUTH_ONLY_DOMAIN', '').lower() user_domain = user.email.split('@')[1].strip().lower() @@ -398,7 +398,11 @@ def login_user(request): return response except AuthFailedError as error: log.exception(error.get_response()) - response = JsonResponse(error.get_response()) + # original code returned a 200 status code with status=False for errors. This flag + # is used for rolling out a transition to using a 400 status code for errors, which + # is a breaking-change, but will hopefully be a tolerable breaking-change. + status = 400 if UPDATE_LOGIN_USER_ERROR_STATUS_CODE.is_enabled() else 200 + response = JsonResponse(error.get_response(), status=status) set_custom_metric('login_user_auth_failed_error', True) set_custom_metric('login_user_response_status', response.status_code) return response @@ -608,9 +612,12 @@ def shim_student_view(view_func, check_logged_in=False): # If an error condition occurs, send a status 400 elif response.status_code != 200 or not success: - # The student views tend to send status 200 even when an error occurs + # 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 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 13e9ab8554..c372192488 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -32,8 +32,8 @@ 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, - authn_waffle + UPDATE_LOGIN_USER_ERROR_STATUS_CODE, + ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY ) from openedx.core.djangoapps.user_authn.tests.utils import setup_login_oauth_client from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms @@ -86,10 +86,12 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): self._assert_audit_log(mock_audit_log, 'info', [u'Login success', self.user_email]) @patch.dict("django.conf.settings.FEATURES", {'SQUELCH_PII_IN_LOGS': True}) - def test_login_success_no_pii(self): - response, mock_audit_log = self._login_response( - self.user_email, self.password, patched_audit_log='student.models.AUDIT_LOG' - ) + @ddt.data(True, False) + def test_login_success_no_pii(self, is_error_status_code_enabled): + with UPDATE_LOGIN_USER_ERROR_STATUS_CODE.override(is_error_status_code_enabled): + response, mock_audit_log = self._login_response( + self.user_email, self.password, patched_audit_log='student.models.AUDIT_LOG' + ) self._assert_response(response, success=True) self._assert_audit_log(mock_audit_log, 'info', [u'Login success']) self._assert_not_in_audit_log(mock_audit_log, 'info', [self.user_email]) @@ -118,13 +120,21 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): self.user.refresh_from_db() assert old_last_login == self.user.last_login - def test_login_fail_no_user_exists(self): + @ddt.data( + (True, 400), + (False, 200), + ) + @ddt.unpack + def test_login_fail_no_user_exists(self, is_error_status_code_enabled, expected_status_code): nonexistent_email = u'not_a_user@edx.org' - response, mock_audit_log = self._login_response( - nonexistent_email, - self.password, + with UPDATE_LOGIN_USER_ERROR_STATUS_CODE.override(is_error_status_code_enabled): + response, mock_audit_log = self._login_response( + nonexistent_email, + self.password, + ) + self._assert_response( + response, success=False, value=self.LOGIN_FAILED_WARNING, status_code=expected_status_code ) - self._assert_response(response, success=False, value=self.LOGIN_FAILED_WARNING) self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', u'Unknown user email', nonexistent_email]) @patch.dict("django.conf.settings.FEATURES", {'SQUELCH_PII_IN_LOGS': True}) @@ -500,9 +510,9 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): result = self.client.post(self.url, post_params) return result, mock_audit_log - def _assert_response(self, response, success=None, value=None): + def _assert_response(self, response, success=None, value=None, status_code=None): """ - Assert that the response had status 200 and returned a valid + Assert that the response has the expected status code and returned a valid JSON-parseable dict. If success is provided, assert that the response had that @@ -511,7 +521,8 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): If value is provided, assert that the response contained that value for 'value' in the JSON dict. """ - self.assertEqual(response.status_code, 200) + expected_status_code = status_code or 200 + self.assertEqual(response.status_code, expected_status_code) try: response_dict = json.loads(response.content.decode('utf-8')) @@ -616,7 +627,7 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): else: AllowedAuthUser.objects.filter(site=site, email=user.email).delete() - with authn_waffle.override(ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY, switch_enabled): + with ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY.override(switch_enabled): value = None if success else u'As an {0} user, You must login with your {0} {1} account.'.format( allowed_domain, provider