Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #22379 from edx/robrap/ARCH-1253-login-post-clean-up

Browse Source 
ARCH-1253: switch login_user errors to 400
This commit is contained in:
Robert Raposa
2019-11-25 16:15:02 -05:00
committed by GitHub
parent d707899d68 5aa6181f85
commit 6086e37c28
4 changed files with 68 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

36
openedx/core/djangoapps/user_authn/config/waffle.py
View File

@@ -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'
)

1
openedx/core/djangoapps/user_authn/urls_common.py
View File

@@ -38,7 +38,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<error>[^/]*)$', login.login_user),
# Moved from user_api/legacy_urls.py
# `user_api` prefix is preserved for backwards compatibility.

15
openedx/core/djangoapps/user_authn/views/login.py
View File

@@ -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

41
openedx/core/djangoapps/user_authn/views/tests/test_login.py
View File

@@ -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