From 9f287165bdc2c6e94edd4bfd624fd46606899e03 Mon Sep 17 00:00:00 2001 From: Gabe Mulley Date: Thu, 30 Apr 2020 10:52:54 -0400 Subject: [PATCH] log registration failures --- .../djangoapps/user_authn/views/register.py | 54 ++++++++++++++++--- .../user_authn/views/tests/test_register.py | 40 +++++++++++++- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/views/register.py b/openedx/core/djangoapps/user_authn/views/register.py index e2dccbbf51..ea2dab4133 100644 --- a/openedx/core/djangoapps/user_authn/views/register.py +++ b/openedx/core/djangoapps/user_authn/views/register.py @@ -56,6 +56,7 @@ from openedx.core.djangoapps.user_authn.views.registration_form import ( AccountCreationForm, RegistrationFormFactory ) +from openedx.core.djangoapps.waffle_utils import WaffleFlag, WaffleFlagNamespace from student.helpers import ( authenticate_new_user, create_or_set_user_attribute_created_on_site, @@ -94,6 +95,24 @@ REGISTRATION_UTM_CREATED_AT = 'registration_utm_created_at' REGISTER_USER = Signal(providing_args=["user", "registration"]) +# .. feature_toggle_name: registration.enable_failure_logging +# .. feature_toggle_type: flag +# .. feature_toggle_default: False +# .. feature_toggle_description: Enable verbose logging of registration failure messages +# .. feature_toggle_category: registration +# .. feature_toggle_use_cases: monitored_rollout +# .. feature_toggle_creation_date: 2020-04-30 +# .. feature_toggle_expiration_date: 2020-06-01 +# .. feature_toggle_warnings: None +# .. feature_toggle_tickets: None +# .. feature_toggle_status: supported +REGISTRATION_FAILURE_LOGGING_FLAG = WaffleFlag( + waffle_namespace=WaffleFlagNamespace(name=u'registration'), + flag_name=u'enable_failure_logging', + flag_undefined_default=False +) + + @transaction.non_atomic_requests def create_account_with_params(request, params): """ @@ -463,7 +482,7 @@ class RegistrationView(APIView): data = request.POST.copy() self._handle_terms_of_service(data) - response = self._handle_duplicate_email_username(data) + response = self._handle_duplicate_email_username(request, data) if response: return response @@ -471,11 +490,11 @@ class RegistrationView(APIView): if response: return response - response = self._create_response({}, status_code=200) + response = self._create_response(request, {}, status_code=200) set_logged_in_cookies(request, response, user) return response - def _handle_duplicate_email_username(self, data): + def _handle_duplicate_email_username(self, request, data): # pylint: disable=no-member # TODO Verify whether this check is needed here - it may be duplicated in user_api. email = data.get('email') @@ -489,7 +508,7 @@ class RegistrationView(APIView): errors["username"] = [{"user_message": accounts_settings.USERNAME_CONFLICT_MSG.format(username=username)}] if errors: - return self._create_response(errors, status_code=409) + return self._create_response(request, errors, status_code=409) def _handle_terms_of_service(self, data): # Backwards compatibility: the student view expects both @@ -510,7 +529,7 @@ class RegistrationView(APIView): errors = { err.field: [{"user_message": text_type(err)}] } - response = self._create_response(errors, status_code=409) + response = self._create_response(request, errors, status_code=409) except ValidationError as err: # Should only get field errors from this exception assert NON_FIELD_ERRORS not in err.message_dict @@ -519,18 +538,39 @@ class RegistrationView(APIView): field: [{"user_message": error} for error in error_list] for field, error_list in err.message_dict.items() } - response = self._create_response(errors, status_code=400) + response = self._create_response(request, errors, status_code=400) except PermissionDenied: response = HttpResponseForbidden(_("Account creation not allowed.")) return response, user - def _create_response(self, response_dict, status_code): + def _create_response(self, request, response_dict, status_code): if status_code == 200: # keeping this `success` field in for now, as we have outstanding clients expecting this response_dict['success'] = True + else: + self._log_validation_errors(request, response_dict, status_code) + return JsonResponse(response_dict, status=status_code) + def _log_validation_errors(self, request, errors, status_code): + if not REGISTRATION_FAILURE_LOGGING_FLAG.is_enabled(): + return + + try: + for field_key, errors in errors.items(): + for error in errors: + log.info( + 'message=registration_failed, status_code=%d, agent="%s", field="%s", error="%s"', + status_code, + request.META.get('HTTP_USER_AGENT', ''), + field_key, + error['user_message'] + ) + except: # pylint: disable=bare-except + log.exception("Failed to log registration validation error") + pass + class RegistrationValidationThrottle(AnonRateThrottle): """ diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_register.py b/openedx/core/djangoapps/user_authn/views/tests/test_register.py index 614b8c746b..c890fae8b2 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_register.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_register.py @@ -50,7 +50,9 @@ from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( from openedx.core.djangoapps.user_api.tests.test_helpers import TestCaseForm from openedx.core.djangoapps.user_api.tests.test_constants import SORTED_COUNTRIES from openedx.core.djangoapps.user_api.tests.test_views import UserAPITestCase -from openedx.core.djangoapps.user_authn.views.register import RegistrationValidationThrottle +from openedx.core.djangoapps.user_authn.views.register import RegistrationValidationThrottle, \ + REGISTRATION_FAILURE_LOGGING_FLAG +from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from openedx.core.lib.api import test_utils from student.helpers import authenticate_new_user @@ -209,6 +211,42 @@ class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCa } ) + @override_waffle_flag(REGISTRATION_FAILURE_LOGGING_FLAG, True) + def test_registration_failure_logging(self): + # Register a user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertHttpOK(response) + + # Try to create a second user with the same email address + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": "Someone Else", + "username": "someone_else", + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertEqual(response.status_code, 409) + response_json = json.loads(response.content.decode('utf-8')) + self.assertDictEqual( + response_json, + { + "email": [{ + "user_message": ( + u"It looks like {} belongs to an existing account. " + "Try again with a different email address." + ).format( + self.EMAIL + ) + }] + } + ) + def test_register_duplicate_username_account_validation_error(self): # Register the first user response = self.client.post(self.url, {