Skip to content

Return the full name of the student as part of the OpenId response. #1941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dianakhuang merged 2 commits into from
Dec 13, 2013
Merged

Return the full name of the student as part of the OpenId response. #1941

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0057f46
Return the full name of the student as part of the OpenId response.
dianakhuang Dec 12, 2013
68b7508
Clean up test conditions.
dianakhuang Dec 13, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 52 additions & 10 deletions common/djangoapps/external_auth/tests/test_openid_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand All @@ -93,6 +96,7 @@ def test_begin_login_with_xrds_url(self):
# 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
Expand All @@ -101,7 +105,8 @@ def test_begin_login_with_xrds_url(self):
.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
Expand Down Expand Up @@ -184,20 +189,23 @@ def attempt_login(self, expected_code, **kwargs):
.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")
Expand Down Expand Up @@ -225,14 +233,16 @@ def _send_bad_redirection_login(self):
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
Expand All @@ -245,6 +255,38 @@ def test_login_openid_handle_redirection_ratelimited(self):
# clear the ratelimit cache so that we don't fail other logins
cache.clear()

@skipUnless(settings.FEATURES.get('AUTH_USE_OPENID') or

@brianhw brianhw Dec 12, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test uses the provider, but doesn't look like it should run if AUTH_USE_OPENID is set but the PROVIDER is not set.

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', '☃']:

@brianhw brianhw Dec 12, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest using unicode backslash notation for this, rather than changing the file encoding to utf-8.

@dianakhuang dianakhuang Dec 13, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I just like the little unicode snowman staring back at me.

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):
"""
Expand All @@ -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.
Expand Down Expand Up @@ -289,4 +332,3 @@ def tearDownClass(cls):
super(OpenIdProviderLiveServerTest, cls).tearDownClass()
except RuntimeError:
print "Warning: Could not shut down test server."
pass
13 changes: 1 addition & 12 deletions common/djangoapps/external_auth/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,

@brianhw brianhw Dec 12, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the original reason for using username instead of profile.name. At first I thought maybe the concern would be that the profile.name might be zero-length. But I wonder if the real motivation was because they were pulling out this value and then storing it for use elsewhere to be the login identifier that would be passed back in later. Don't know, but is there any chance that this might be zero-length?

@dianakhuang dianakhuang Dec 13, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't let people register with a 0 length name, and we don't let them change to a 0 length name later, so this should be fine from that perspective.

I think originally there was some paranoia about privacy concerns around sending across the fullname, but I guess now we're willing to compromise by warning people about what information we're sending over.

@brianhw brianhw Dec 13, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks for checking.

}

# the request succeeded:
Expand Down
1 change: 1 addition & 0 deletions lms/templates/provider_login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ <h2>${_("Log In")}</h2>
%if error:
<div id="login_error" class="modal-form-error" style="display: block;">${_("Email or password is incorrect.")}</div>
%endif
<p>${_("Please note that we will be sending your user name, email, and full name to this third party site.")}</p>
<label>${_("E-mail")}</label>
<input type="text" name="email" placeholder="${_('E-mail')}" tabindex="1" autofocus="autofocus" />
<label>${_("Password")}</label>
Expand Down