From f33bfd1c6c07210eed3cc1975f7e55c68c594a85 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Fri, 28 Jun 2013 14:41:06 -0400 Subject: [PATCH] Address code review feedback --- .../tests/test_openid_provider.py | 96 +++++-------------- common/djangoapps/external_auth/views.py | 47 +++++---- 2 files changed, 49 insertions(+), 94 deletions(-) diff --git a/common/djangoapps/external_auth/tests/test_openid_provider.py b/common/djangoapps/external_auth/tests/test_openid_provider.py index f62a6d3f14..1f7f201087 100644 --- a/common/djangoapps/external_auth/tests/test_openid_provider.py +++ b/common/djangoapps/external_auth/tests/test_openid_provider.py @@ -13,6 +13,7 @@ from django.test.utils import override_settings # from django.contrib.auth.models import User from django.core.urlresolvers import reverse from django.test.client import RequestFactory +from unittest import skipUnless class MyFetcher(HTTPFetcher): @@ -68,10 +69,9 @@ class OpenIdProviderTest(TestCase): Tests of the OpenId login """ + @skipUnless(settings.MITX_FEATURES.get('AUTH_USE_OPENID') or + settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) def test_begin_login_with_xrds_url(self): - # skip the test if openid is not enabled (as in cms.envs.test): - if not settings.MITX_FEATURES.get('AUTH_USE_OPENID') or not settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'): - return # the provider URL must be converted to an absolute URL in order to be # used as an openid provider. @@ -97,10 +97,9 @@ class OpenIdProviderTest(TestCase): "got code {0} for url '{1}'. Expected code {2}" .format(resp.status_code, url, code)) + @skipUnless(settings.MITX_FEATURES.get('AUTH_USE_OPENID') or + settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) def test_begin_login_with_login_url(self): - # skip the test if openid is not enabled (as in cms.envs.test): - if not settings.MITX_FEATURES.get('AUTH_USE_OPENID') or not settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'): - return # the provider URL must be converted to an absolute URL in order to be # used as an openid provider. @@ -148,9 +147,8 @@ class OpenIdProviderTest(TestCase): # # - def test_open_id_setup(self): - if not settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'): - return + def attempt_login(self, expected_code, **kwargs): + """ Attempt to log in through the open id provider login """ url = reverse('openid-provider-login') post_args = { "openid.mode": "checkid_setup", @@ -172,74 +170,34 @@ class OpenIdProviderTest(TestCase): "openid.ax.type.old_nickname": "http://schema.openid.net/namePerson/friendly", "openid.ax.type.old_fullname": "http://schema.openid.net/namePerson", } + # override the default args with any given arguments + for key in kwargs: + post_args["openid." + key] = kwargs[key] + resp = self.client.post(url, post_args) - code = 200 + code = expected_code self.assertEqual(resp.status_code, code, "got code {0} for url '{1}'. Expected code {2}" .format(resp.status_code, url, code)) + @skipUnless(settings.MITX_FEATURES.get('AUTH_USE_OPENID') or + settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) + def test_open_id_setup(self): + """ Attempt a standard successful login """ + self.attempt_login(200) + + @skipUnless(settings.MITX_FEATURES.get('AUTH_USE_OPENID') or + settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) def test_invalid_namespace(self): """ Test for 403 error code when the namespace of the request is invalid""" - if not settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'): - return - url = reverse('openid-provider-login') - post_args = { - "openid.mode": "checkid_setup", - "openid.return_to": "http://testserver/openid/complete/?janrain_nonce=2013-01-23T06%3A20%3A17ZaN7j6H", - "openid.assoc_handle": "{HMAC-SHA1}{50ff8120}{rh87+Q==}", - "openid.claimed_id": "http://specs.openid.net/auth/2.0/identifier_select", - "openid.ns": "http%3A%2F%2Fspecs.openid.net%2Fauth%2F2.0", - "openid.realm": "http://testserver/", - "openid.identity": "http://specs.openid.net/auth/2.0/identifier_select", - "openid.ns.ax": "http://openid.net/srv/ax/1.0", - "openid.ax.mode": "fetch_request", - "openid.ax.required": "email,fullname,old_email,firstname,old_nickname,lastname,old_fullname,nickname", - "openid.ax.type.fullname": "http://axschema.org/namePerson", - "openid.ax.type.lastname": "http://axschema.org/namePerson/last", - "openid.ax.type.firstname": "http://axschema.org/namePerson/first", - "openid.ax.type.nickname": "http://axschema.org/namePerson/friendly", - "openid.ax.type.email": "http://axschema.org/contact/email", - "openid.ax.type.old_email": "http://schema.openid.net/contact/email", - "openid.ax.type.old_nickname": "http://schema.openid.net/namePerson/friendly", - "openid.ax.type.old_fullname": "http://schema.openid.net/namePerson", - } - resp = self.client.post(url, post_args) - code = 403 - self.assertEqual(resp.status_code, code, - "got code {0} for url '{1}'. Expected code {2}" - .format(resp.status_code, url, code)) + 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.MITX_FEATURES.get('AUTH_USE_OPENID') or + settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) def test_invalid_return_url(self): """ Test for 403 error code when the url""" - if not settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'): - return - url = reverse('openid-provider-login') - post_args = { - "openid.mode": "checkid_setup", - "openid.return_to": "http://apps.cs50.edx.or", - "openid.assoc_handle": "{HMAC-SHA1}{50ff8120}{rh87+Q==}", - "openid.claimed_id": "http://specs.openid.net/auth/2.0/identifier_select", - "openid.ns": "http://specs.openid.net/auth/2.0", - "openid.realm": "http://testserver/", - "openid.identity": "http://specs.openid.net/auth/2.0/identifier_select", - "openid.ns.ax": "http://openid.net/srv/ax/1.0", - "openid.ax.mode": "fetch_request", - "openid.ax.required": "email,fullname,old_email,firstname,old_nickname,lastname,old_fullname,nickname", - "openid.ax.type.fullname": "http://axschema.org/namePerson", - "openid.ax.type.lastname": "http://axschema.org/namePerson/last", - "openid.ax.type.firstname": "http://axschema.org/namePerson/first", - "openid.ax.type.nickname": "http://axschema.org/namePerson/friendly", - "openid.ax.type.email": "http://axschema.org/contact/email", - "openid.ax.type.old_email": "http://schema.openid.net/contact/email", - "openid.ax.type.old_nickname": "http://schema.openid.net/namePerson/friendly", - "openid.ax.type.old_fullname": "http://schema.openid.net/namePerson", - } - resp = self.client.post(url, post_args) - code = 403 - self.assertEqual(resp.status_code, code, - "got code {0} for url '{1}'. Expected code {2}" - .format(resp.status_code, url, code)) + self.attempt_login(403, return_to="http://apps.cs50.edx.or") class OpenIdProviderLiveServerTest(LiveServerTestCase): @@ -250,11 +208,9 @@ class OpenIdProviderLiveServerTest(LiveServerTestCase): Here we do the former. """ + @skipUnless(settings.MITX_FEATURES.get('AUTH_USE_OPENID') or + settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'), True) def test_begin_login(self): - # skip the test if openid is not enabled (as in cms.envs.test): - if not settings.MITX_FEATURES.get('AUTH_USE_OPENID') or not settings.MITX_FEATURES.get('AUTH_USE_OPENID_PROVIDER'): - return - # the provider URL must be converted to an absolute URL in order to be # used as an openid provider. provider_url = reverse('openid-provider-xrds') diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 34d65073f7..2a673acdf8 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -102,7 +102,7 @@ def openid_login_complete(request, oid_backend = openid_auth.OpenIDBackend() details = oid_backend._extract_user_details(openid_response) - log.debug('openid success, details={0}'.format(details)) + log.debug('openid success, details=%s', details) url = getattr(settings, 'OPENID_SSO_SERVER_URL', None) external_domain = "openid:%s" % url @@ -132,7 +132,7 @@ def external_login_or_signup(request, try: eamap = ExternalAuthMap.objects.get(external_id=external_id, external_domain=external_domain) - log.debug('Found eamap={0}'.format(eamap)) + log.debug('Found eamap=%s', eamap) except ExternalAuthMap.DoesNotExist: # go render form for creating edX user eamap = ExternalAuthMap(external_id=external_id, @@ -141,11 +141,11 @@ def external_login_or_signup(request, eamap.external_email = email eamap.external_name = fullname eamap.internal_password = generate_password() - log.debug('Created eamap={0}'.format(eamap)) + log.debug('Created eamap=%s', eamap) eamap.save() - log.info(u"External_Auth login_or_signup for {0} : {1} : {2} : {3}".format(external_domain, external_id, email, fullname)) + log.info(u"External_Auth login_or_signup for %s : %s : %s : %s", external_domain, external_id, email, fullname) internal_user = eamap.user if internal_user is None: if settings.MITX_FEATURES.get('AUTH_USE_SHIB'): @@ -157,7 +157,7 @@ def external_login_or_signup(request, eamap.user = link_user eamap.save() internal_user = link_user - log.info('SHIB: Linking existing account for {0}'.format(eamap.external_email)) + log.info('SHIB: Linking existing account for %s', eamap.external_email) # now pass through to log in else: # otherwise, there must have been an error, b/c we've already linked a user with these external @@ -168,10 +168,10 @@ def external_login_or_signup(request, % getattr(settings, 'TECH_SUPPORT_EMAIL', 'techsupport@class.stanford.edu'))) return default_render_failure(request, failure_msg) except User.DoesNotExist: - log.info('SHIB: No user for {0} yet, doing signup'.format(eamap.external_email)) + log.info('SHIB: No user for %s yet, doing signup', eamap.external_email) return signup(request, eamap) else: - log.info('No user for {0} yet.formatdoing signup'.format(eamap.external_email)) + log.info('No user for %s yet. doing signup', eamap.external_email) return signup(request, eamap) # We trust shib's authentication, so no need to authenticate using the password again @@ -183,17 +183,17 @@ def external_login_or_signup(request, else: auth_backend = 'django.contrib.auth.backends.ModelBackend' user.backend = auth_backend - log.info('SHIB: Logging in linked user {0}'.format(user.email)) + log.info('SHIB: Logging in linked user %s', user.email) else: uname = internal_user.username user = authenticate(username=uname, password=eamap.internal_password) if user is None: - log.warning("External Auth Login failed for {0} / {1}".format( - uname, eamap.internal_password)) + log.warning("External Auth Login failed for %s / %s", + uname, eamap.internal_password) return signup(request, eamap) if not user.is_active: - log.warning("User {0} is not active".format(uname)) + log.warning("User %s is not active", uname) # TODO: improve error page msg = 'Account not yet activated: please look for link in your email' return default_render_failure(request, msg) @@ -208,7 +208,7 @@ def external_login_or_signup(request, student_views.try_change_enrollment(enroll_request) else: student_views.try_change_enrollment(request) - log.info("Login success - {0} ({1})".format(user.username, user.email)) + log.info("Login success - %s (%s)", user.username, user.email) if retfun is None: return redirect('/') return retfun() @@ -261,7 +261,7 @@ def signup(request, eamap=None): except ValidationError: context['ask_for_email'] = True - log.info('EXTAUTH: Doing signup for {0}'.format(eamap.external_id)) + log.info('EXTAUTH: Doing signup for %s', eamap.external_id) return student_views.register_user(request, extra_context=context) @@ -405,7 +405,7 @@ def shib_login(request): shib['sn'] = shib['sn'].split(";")[0].strip().capitalize().decode('utf-8') shib['givenName'] = shib['givenName'].split(";")[0].strip().capitalize().decode('utf-8') - log.info("SHIB creds returned: {0}".format(shib)) + log.info("SHIB creds returned: %r", shib) return external_login_or_signup(request, external_id=shib['REMOTE_USER'], @@ -643,7 +643,7 @@ def provider_login(request): try: openid_request = server.decodeRequest(querydict) except (UntrustedReturnURL, ProtocolError): - return default_render_failure(request, "Invalid OpenID request") + openid_request = None if not openid_request: return default_render_failure(request, "Invalid OpenID request") @@ -700,8 +700,8 @@ def provider_login(request): user = User.objects.get(email=email) except User.DoesNotExist: request.session['openid_error'] = True - msg = "OpenID login failed - Unknown user email: {0}".format(email) - log.warning(msg) + msg = "OpenID login failed - Unknown user email: %s" + log.warning(msg, email) return HttpResponseRedirect(openid_request_url) # attempt to authenticate user (but not actually log them in...) @@ -711,9 +711,8 @@ def provider_login(request): user = authenticate(username=username, password=password) if user is None: request.session['openid_error'] = True - msg = "OpenID login failed - password for {0} is invalid" - msg = msg.format(email) - log.warning(msg) + msg = "OpenID login failed - password for %s is invalid" + log.warning(msg, email) return HttpResponseRedirect(openid_request_url) # authentication succeeded, so fetch user information @@ -723,8 +722,8 @@ def provider_login(request): if 'openid_error' in request.session: del request.session['openid_error'] - log.info("OpenID login success - {0} ({1})".format(user.username, - user.email)) + log.info("OpenID login success - %s (%s)", + user.username, user.email) # redirect user to return_to location url = endpoint + urlquote(user.username) @@ -754,8 +753,8 @@ def provider_login(request): # the account is not active, so redirect back to the login page: request.session['openid_error'] = True - msg = "Login failed - Account not active for user {0}".format(username) - log.warning(msg) + msg = "Login failed - Account not active for user %s" + log.warning(msg, username) return HttpResponseRedirect(openid_request_url) # determine consumer domain if applicable