Skip to content
Snippets Groups Projects
Commit f33bfd1c authored by Diana Huang's avatar Diana Huang
Browse files

Address code review feedback

parent c98651fa
No related merge requests found
Hide whitespace changes
Inline Side-by-side
common/djangoapps/external_auth/tests/test_openid_provider.py
+ 26
− 70
View file @ f33bfd1c
......@@ -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):
# <input name="openid.return_to" type="hidden" value="http://testserver/openid/complete/?janrain_nonce=2013-01-23T06%3A20%3A17ZaN7j6H" />
# <input name="openid.assoc_handle" type="hidden" value="{HMAC-SHA1}{50ff8120}{rh87+Q==}" />
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')
......
common/djangoapps/external_auth/views.py
+ 23
− 24
View file @ f33bfd1c
......@@ -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
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment