From 0057f460ec6ba49a7830f119c554a7f93bf852a3 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Thu, 12 Dec 2013 11:52:37 -0500 Subject: [PATCH 1/2] Return the full name of the student as part of the OpenId response. LMS-750 --- .../tests/test_openid_provider.py | 62 ++++++++++++++++--- common/djangoapps/external_auth/views.py | 13 +--- lms/templates/provider_login.html | 1 + 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/common/djangoapps/external_auth/tests/test_openid_provider.py b/common/djangoapps/external_auth/tests/test_openid_provider.py index cb7d0c0c14..3bea52b019 100644 --- a/common/djangoapps/external_auth/tests/test_openid_provider.py +++ b/common/djangoapps/external_auth/tests/test_openid_provider.py @@ -1,11 +1,13 @@ +#-*- encoding=utf-8 -*- ''' Created on Jan 18, 2013 @author: brian ''' import openid +import json from openid.fetchers import HTTPFetcher, HTTPResponse -from urlparse import parse_qs +from urlparse import parse_qs, urlparse from django.conf import settings from django.test import TestCase, LiveServerTestCase @@ -73,7 +75,8 @@ class OpenIdProviderTest(TestCase): """ @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_begin_login_with_xrds_url(self): # the provider URL must be converted to an absolute URL in order to be @@ -93,6 +96,7 @@ class OpenIdProviderTest(TestCase): # now we can begin the login process by invoking a local openid client, # with a pointer to the (also-local) openid provider: with self.settings(OPENID_SSO_SERVER_URL=abs_provider_url): + url = reverse('openid-login') resp = self.client.post(url) code = 200 @@ -101,7 +105,8 @@ class OpenIdProviderTest(TestCase): .format(resp.status_code, url, code)) @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_begin_login_with_login_url(self): # the provider URL must be converted to an absolute URL in order to be @@ -184,20 +189,23 @@ class OpenIdProviderTest(TestCase): .format(resp.status_code, url, code)) @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_open_id_setup(self): """ Attempt a standard successful login """ self.attempt_login(200) @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_invalid_namespace(self): """ Test for 403 error code when the namespace of the request is invalid""" self.attempt_login(403, ns="http%3A%2F%2Fspecs.openid.net%2Fauth%2F2.0") @override_settings(OPENID_PROVIDER_TRUSTED_ROOTS=['http://apps.cs50.edx.org']) @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_invalid_return_url(self): """ Test for 403 error code when the url""" self.attempt_login(403, return_to="http://apps.cs50.edx.or") @@ -225,14 +233,16 @@ class OpenIdProviderTest(TestCase): return response @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_login_openid_handle_redirection(self): """ Test to see that we can handle login redirection properly""" response = self._send_bad_redirection_login() self.assertEquals(response.status_code, 302) @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_login_openid_handle_redirection_ratelimited(self): # try logging in 30 times, the default limit in the number of failed # log in attempts before the rate gets limited @@ -245,6 +255,38 @@ class OpenIdProviderTest(TestCase): # clear the ratelimit cache so that we don't fail other logins cache.clear() + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + 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) + # all information is embedded in the redirect url + location = resp['Location'] + # parse the url + 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.ext1.1'][0], user.email) + self.assertEquals(parsed_qs['openid.ax.value.ext0.1'][0], user.profile.name) + class OpenIdProviderLiveServerTest(LiveServerTestCase): """ @@ -255,7 +297,8 @@ class OpenIdProviderLiveServerTest(LiveServerTestCase): """ @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + 'OpenID not enabled') def test_begin_login(self): # the provider URL must be converted to an absolute URL in order to be # used as an openid provider. @@ -289,4 +332,3 @@ class OpenIdProviderLiveServerTest(LiveServerTestCase): super(OpenIdProviderLiveServerTest, cls).tearDownClass() except RuntimeError: print "Warning: Could not shut down test server." - pass diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 42d0f2bf89..1c44b7455b 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -818,23 +818,12 @@ def provider_login(request): url = endpoint + urlquote(user.username) response = openid_request.answer(True, None, url) - # TODO: for CS50 we are forcibly returning the username - # instead of fullname. In the OpenID simple registration - # extension, we don't have to return any fields we don't - # want to, even if they were marked as required by the - # Consumer. The behavior of what to do when there are - # missing fields is up to the Consumer. The proper change - # should only return the username, however this will likely - # break the CS50 client. Temporarily we will be returning - # username filling in for fullname in addition to username - # as sreg nickname. - # Note too that this is hardcoded, and not really responding to # the extensions that were registered in the first place. results = { 'nickname': user.username, 'email': user.email, - 'fullname': user.username + 'fullname': user.profile.name, } # the request succeeded: diff --git a/lms/templates/provider_login.html b/lms/templates/provider_login.html index 3bcd22fafd..b7f7067bef 100644 --- a/lms/templates/provider_login.html +++ b/lms/templates/provider_login.html @@ -41,6 +41,7 @@ %if error: %endif +

${_("Please note that we will be sending your user name, email, and full name to this third party site.")}

From 68b75086f25872df68ef015cac76478afd8dd29d Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Fri, 13 Dec 2013 09:11:00 -0500 Subject: [PATCH 2/2] Clean up test conditions. LMS-750 --- .../tests/test_openid_provider.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/external_auth/tests/test_openid_provider.py b/common/djangoapps/external_auth/tests/test_openid_provider.py index 3bea52b019..0a610cb892 100644 --- a/common/djangoapps/external_auth/tests/test_openid_provider.py +++ b/common/djangoapps/external_auth/tests/test_openid_provider.py @@ -74,7 +74,7 @@ class OpenIdProviderTest(TestCase): Tests of the OpenId login """ - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_begin_login_with_xrds_url(self): @@ -104,7 +104,7 @@ class OpenIdProviderTest(TestCase): "got code {0} for url '{1}'. Expected code {2}" .format(resp.status_code, url, code)) - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_begin_login_with_login_url(self): @@ -188,14 +188,14 @@ class OpenIdProviderTest(TestCase): "got code {0} for url '{1}'. Expected code {2}" .format(resp.status_code, url, code)) - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_open_id_setup(self): """ Attempt a standard successful login """ self.attempt_login(200) - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_invalid_namespace(self): @@ -203,7 +203,7 @@ class OpenIdProviderTest(TestCase): self.attempt_login(403, ns="http%3A%2F%2Fspecs.openid.net%2Fauth%2F2.0") @override_settings(OPENID_PROVIDER_TRUSTED_ROOTS=['http://apps.cs50.edx.org']) - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_invalid_return_url(self): @@ -232,7 +232,7 @@ class OpenIdProviderTest(TestCase): response = provider_login(request) return response - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_login_openid_handle_redirection(self): @@ -240,7 +240,7 @@ class OpenIdProviderTest(TestCase): response = self._send_bad_redirection_login() self.assertEquals(response.status_code, 302) - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_login_openid_handle_redirection_ratelimited(self): @@ -255,8 +255,7 @@ class OpenIdProviderTest(TestCase): # clear the ratelimit cache so that we don't fail other logins cache.clear() - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or - settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_openid_final_response(self): @@ -296,7 +295,7 @@ class OpenIdProviderLiveServerTest(LiveServerTestCase): Here we do the former. """ - @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or + @skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') and settings.FEATURES.get('AUTH_USE_OPENID_PROVIDER'), 'OpenID not enabled') def test_begin_login(self):