From 451d70a2d17dee601a3b57a5d0de41f8661f6752 Mon Sep 17 00:00:00 2001 From: Jacek Bzdak Date: Mon, 30 Nov 2015 14:19:14 +0100 Subject: [PATCH 1/2] Added Jacek Bzdak to AUTHORS file. --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index c04dc2c40e..8026729689 100644 --- a/AUTHORS +++ b/AUTHORS @@ -256,3 +256,4 @@ Awais Jibran Muhammad Rehan Shawn Milochik Afeef Janjua +Jacek Bzdak From 3506d962f8a6a1b39294035851f83c388865e451 Mon Sep 17 00:00:00 2001 From: Jacek Bzdak Date: Sun, 29 Nov 2015 11:11:09 +0100 Subject: [PATCH 2/2] Fix for error in TNL-363 Audit logs were causing open_id provider logins to fail when username or e-mail address contained unicode data. --- .../tests/test_openid_provider.py | 99 ++++++++++++++++--- common/djangoapps/external_auth/views.py | 26 ++--- 2 files changed, 99 insertions(+), 26 deletions(-) diff --git a/common/djangoapps/external_auth/tests/test_openid_provider.py b/common/djangoapps/external_auth/tests/test_openid_provider.py index 21b2789aca..419bab3f35 100644 --- a/common/djangoapps/external_auth/tests/test_openid_provider.py +++ b/common/djangoapps/external_auth/tests/test_openid_provider.py @@ -260,27 +260,100 @@ class OpenIdProviderTest(TestCase): # clear the ratelimit cache so that we don't fail other logins cache.clear() + def _attempt_login_and_perform_final_response(self, user, profile_name): + """ + Performs full procedure of a successful OpenID provider login for user, + all required data is taken form ``user`` attribute which is an instance + of ``User`` model. As a convenience this method will also set + ``profile.name`` for the user. + """ + url = reverse('openid-provider-login') + + # login to the client so that we can persist session information + user.profile.name = profile_name + user.profile.save() + # It is asssumed that user's password is test (default for UserFactory) + self.client.login(username=user.username, password='test') + # login once to get the right session information + self.attempt_login(200) + post_args = { + 'email': user.email, + 'password': 'test' + } + + # call url again, this time with username and password + return self.client.post(url, post_args) + + @skipUnless( + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') + def test_provider_login_can_handle_unicode_email(self): + user = UserFactory(email=u"user.ąęł@gmail.com") + resp = self._attempt_login_and_perform_final_response(user, u"Jan ĄĘŁ") + location = resp['Location'] + parsed_url = urlparse(location) + parsed_qs = parse_qs(parsed_url.query) + self.assertEquals(parsed_qs['openid.ax.type.ext1'][0], 'http://axschema.org/contact/email') + self.assertEquals(parsed_qs['openid.ax.type.ext0'][0], 'http://axschema.org/namePerson') + self.assertEquals(parsed_qs['openid.ax.value.ext0.1'][0], + user.profile.name.encode('utf-8')) # pylint: disable=no-member + self.assertEquals(parsed_qs['openid.ax.value.ext1.1'][0], + user.email.encode('utf-8')) # pylint: disable=no-member + + @skipUnless( + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') + def test_provider_login_can_handle_unicode_email_invalid_password(self): + user = UserFactory(email=u"user.ąęł@gmail.com") + url = reverse('openid-provider-login') + + # login to the client so that we can persist session information + user.profile.name = u"Jan ĄĘ" + user.profile.save() + # It is asssumed that user's password is test (default for UserFactory) + self.client.login(username=user.username, password='test') + # login once to get the right session information + self.attempt_login(200) + # We trigger situation where user password is invalid at last phase + # of openid login + post_args = { + 'email': user.email, + 'password': 'invalid-password' + } + + # call url again, this time with username and password + return self.client.post(url, post_args) + + @skipUnless( + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') + def test_provider_login_can_handle_unicode_email_inactive_account(self): + user = UserFactory(email=u"user.ąęł@gmail.com", username=u"ąęół") + url = reverse('openid-provider-login') + + # login to the client so that we can persist session information + user.profile.name = u'Jan ĄĘ' + user.profile.save() # pylint: disable=no-member + self.client.login(username=user.username, password='test') + # login once to get the right session information + self.attempt_login(200) + # We trigger situation where user is not active at final phase of + # OpenId login. + user.is_active = False + user.save() # pylint: disable=no-member + post_args = { + 'email': user.email, + 'password': 'test' + } + # call url again, this time with username and password + self.client.post(url, post_args) + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_openid_final_response(self): - url = reverse('openid-provider-login') user = UserFactory() # login to the client so that we can persist session information for name in ['Robot 33', '☃']: - user.profile.name = name - user.profile.save() - self.client.login(username=user.username, password='test') - # login once to get the right session information - self.attempt_login(200) - post_args = { - 'email': user.email, - 'password': 'test', - } - - # call url again, this time with username and password - resp = self.client.post(url, post_args) + resp = self._attempt_login_and_perform_final_response(user, name) # all information is embedded in the redirect url location = resp['Location'] # parse the url diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index b72f536fbf..a09dd9e3a4 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -836,9 +836,9 @@ def provider_login(request): except User.DoesNotExist: request.session['openid_error'] = True if settings.FEATURES['SQUELCH_PII_IN_LOGS']: - AUDIT_LOG.warning("OpenID login failed - Unknown user email") + AUDIT_LOG.warning(u"OpenID login failed - Unknown user email") else: - msg = "OpenID login failed - Unknown user email: {0}".format(email) + msg = u"OpenID login failed - Unknown user email: {0}".format(email) AUDIT_LOG.warning(msg) return HttpResponseRedirect(openid_request_url) @@ -849,16 +849,16 @@ def provider_login(request): try: user = authenticate(username=username, password=password, request=request) except RateLimitException: - AUDIT_LOG.warning('OpenID - Too many failed login attempts.') + AUDIT_LOG.warning(u'OpenID - Too many failed login attempts.') return HttpResponseRedirect(openid_request_url) if user is None: request.session['openid_error'] = True if settings.FEATURES['SQUELCH_PII_IN_LOGS']: - AUDIT_LOG.warning("OpenID login failed - invalid password") + AUDIT_LOG.warning(u"OpenID login failed - invalid password") else: - msg = "OpenID login failed - password for {0} is invalid".format(email) - AUDIT_LOG.warning(msg) + AUDIT_LOG.warning( + u"OpenID login failed - password for %s is invalid", email) return HttpResponseRedirect(openid_request_url) # authentication succeeded, so fetch user information @@ -869,11 +869,10 @@ def provider_login(request): del request.session['openid_error'] if settings.FEATURES['SQUELCH_PII_IN_LOGS']: - AUDIT_LOG.info("OpenID login success - user.id: {0}".format(user.id)) + AUDIT_LOG.info(u"OpenID login success - user.id: %s", user.id) else: - AUDIT_LOG.info("OpenID login success - {0} ({1})".format( - user.username, user.email)) - + AUDIT_LOG.info( + u"OpenID login success - %s (%s)", user.username, user.email) # redirect user to return_to location url = endpoint + urlquote(user.username) response = openid_request.answer(True, None, url) @@ -892,10 +891,11 @@ def provider_login(request): # the account is not active, so redirect back to the login page: request.session['openid_error'] = True if settings.FEATURES['SQUELCH_PII_IN_LOGS']: - AUDIT_LOG.warning("Login failed - Account not active for user.id {0}".format(user.id)) + AUDIT_LOG.warning( + u"Login failed - Account not active for user.id %s", user.id) else: - msg = "Login failed - Account not active for user {0}".format(username) - AUDIT_LOG.warning(msg) + AUDIT_LOG.warning( + u"Login failed - Account not active for user %s", username) return HttpResponseRedirect(openid_request_url) # determine consumer domain if applicable