Shibboleth Auth #67

Merged
merged 6 commits into from Jun 21, 2013

Conversation

Projects
None yet
5 participants
@jbau

jbau commented Jun 5, 2013

where auth is mostly authentication, but also slightly authorization

common/djangoapps/external_auth/views.py
+ shib_error_msg = """
+ Your university identity server did not return your ID information to us.
+ Please try logging in again. (You may need to restart your browser.)
+ """

This comment has been minimized.

@chrisndodge

chrisndodge Jun 5, 2013

Contributor

Do we have I18N support in LMS? If so, can I suggest we try to use it for any end-user display strings so we don't have to go back and retrofit? Not sure who is the best person to ask about I18N buildout in LMS, @shnayder do you have any info?

@chrisndodge

chrisndodge Jun 5, 2013

Contributor

Do we have I18N support in LMS? If so, can I suggest we try to use it for any end-user display strings so we don't have to go back and retrofit? Not sure who is the best person to ask about I18N buildout in LMS, @shnayder do you have any info?

This comment has been minimized.

@shnayder

shnayder Jun 5, 2013

We haven't done full discovery yet, but in django apps we should wrap
everything in _ (
https://edx-wiki.atlassian.net/wiki/display/ENG/I18n+coding+guidelines) --
we still need to figure out what to do in non-django libraries.

On Wed, Jun 5, 2013 at 10:59 AM, chrisndodge notifications@github.comwrote:

In common/djangoapps/external_auth/views.py:

@@ -304,6 +308,50 @@ def ssl_login(request):
retfun=retfun)

+# -----------------------------------------------------------------------------
+# Shibboleth (Stanford and others. Uses Apache environment variables)
+# -----------------------------------------------------------------------------
+def shib_login(request, retfun=None):
+

  • shib_error_msg = """
  • Your university identity server did not return your ID information to us.
  • Please try logging in again. (You may need to restart your browser.)
  • """

Do we have I18N support in LMS? If so, can I suggest we try to use it for
any end-user display strings so we don't have to go back and retrofit? Not
sure who is the best person to ask about I18N buildout in LMS, @shnayderhttps://github.com/shnayderdo you have any info?


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/67/files#r4546993
.

@shnayder

shnayder Jun 5, 2013

We haven't done full discovery yet, but in django apps we should wrap
everything in _ (
https://edx-wiki.atlassian.net/wiki/display/ENG/I18n+coding+guidelines) --
we still need to figure out what to do in non-django libraries.

On Wed, Jun 5, 2013 at 10:59 AM, chrisndodge notifications@github.comwrote:

In common/djangoapps/external_auth/views.py:

@@ -304,6 +308,50 @@ def ssl_login(request):
retfun=retfun)

+# -----------------------------------------------------------------------------
+# Shibboleth (Stanford and others. Uses Apache environment variables)
+# -----------------------------------------------------------------------------
+def shib_login(request, retfun=None):
+

  • shib_error_msg = """
  • Your university identity server did not return your ID information to us.
  • Please try logging in again. (You may need to restart your browser.)
  • """

Do we have I18N support in LMS? If so, can I suggest we try to use it for
any end-user display strings so we don't have to go back and retrofit? Not
sure who is the best person to ask about I18N buildout in LMS, @shnayderhttps://github.com/shnayderdo you have any info?


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/67/files#r4546993
.

This comment has been minimized.

@cpennington

cpennington Jun 5, 2013

Member

You probably also want to use textwrap.dedent on this, so that you don't have indentation built in to it.

@cpennington

cpennington Jun 5, 2013

Member

You probably also want to use textwrap.dedent on this, so that you don't have indentation built in to it.

This comment has been minimized.

@jbau

jbau Jun 5, 2013

Thanks. all of these are reasonable suggestions. I'll wrap in _ and use dedent

@jbau

jbau Jun 5, 2013

Thanks. all of these are reasonable suggestions. I'll wrap in _ and use dedent

+ ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
+ ('external_id', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)),
+ ('external_domain', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)),
+ ('external_credentials', self.gf('django.db.models.fields.TextField')(blank=True)),

This comment has been minimized.

@chrisndodge

chrisndodge Jun 5, 2013

Contributor

From a security standpoint, what gets stored in here?

@chrisndodge

chrisndodge Jun 5, 2013

Contributor

From a security standpoint, what gets stored in here?

This comment has been minimized.

@jbau

jbau Jun 5, 2013

I don't know what the MIT ssl stores there, but here's my data with Google openid and Stanford shib:

openid:

{"first_name": "Jason", "last_name": "Bau", "nickname": null, "email": "jbau@edx.org"}

shib:

{"REMOTE_USER": "jbau@stanford.edu", "eppn": "jbau@stanford.edu", "Shib-Identity-Provider": "https://idp.stanford.edu/", "sn": "bau", "mail": "jbau@stanford.edu", "givenName": "jason"}

in other words, it's nothing that's not already stored with a normal registration.

@jbau

jbau Jun 5, 2013

I don't know what the MIT ssl stores there, but here's my data with Google openid and Stanford shib:

openid:

{"first_name": "Jason", "last_name": "Bau", "nickname": null, "email": "jbau@edx.org"}

shib:

{"REMOTE_USER": "jbau@stanford.edu", "eppn": "jbau@stanford.edu", "Shib-Identity-Provider": "https://idp.stanford.edu/", "sn": "bau", "mail": "jbau@stanford.edu", "givenName": "jason"}

in other words, it's nothing that's not already stored with a normal registration.

@@ -180,6 +180,8 @@ class CourseFields(object):
checklists = List(scope=Scope.settings)
info_sidebar_name = String(scope=Scope.settings, default='Course Handouts')
show_timezone = Boolean(help="True if timezones should be shown on dates in the courseware", scope=Scope.settings, default=True)
+ restrict_reg = String(help="External login method associated with user accounts allowed to register in course",

This comment has been minimized.

@cpennington

cpennington Jun 5, 2013

Member

This variable is poorly named. This is really the external auth domain to which registration is restricted.

@cpennington

cpennington Jun 5, 2013

Member

This variable is poorly named. This is really the external auth domain to which registration is restricted.

This comment has been minimized.

@jbau

jbau Jun 5, 2013

how about enrollment_domain

@jbau

jbau Jun 5, 2013

how about enrollment_domain

This comment has been minimized.

@jbau

jbau Jun 5, 2013

or enrollment_limited_to

@jbau

jbau Jun 5, 2013

or enrollment_limited_to

This comment has been minimized.

@cpennington

cpennington Jun 5, 2013

Member

I think enrollment_domain is probably best.

@cpennington

cpennington Jun 5, 2013

Member

I think enrollment_domain is probably best.

+ <li class="nav-global-04">
+ <a class="cta cta-register" href="/register">Register Now</a>
+ </li>
+ % endif

This comment has been minimized.

@cpennington

cpennington Jun 5, 2013

Member

This pattern means that every time we enable a new type of external login, we'll also have to enable a new registration link, which is really grungy. It seems to me that we should have a single login link that dispatches to the appropriate backend function depending on the registration method required.

@cpennington

cpennington Jun 5, 2013

Member

This pattern means that every time we enable a new type of external login, we'll also have to enable a new registration link, which is really grungy. It seems to me that we should have a single login link that dispatches to the appropriate backend function depending on the registration method required.

This comment has been minimized.

@jbau

jbau Jun 5, 2013

I agree about the non-maintainability, so will change to a dispatcher pattern.

One ugly requirement w.r.t. shib is that it depends on exact URL locations (these are proxied from nginx -> apache -> mod_shib), so this dispatcher will have to use redirects instead of hooking up appropriate backend functions, at least for shib.

@jbau

jbau Jun 5, 2013

I agree about the non-maintainability, so will change to a dispatcher pattern.

One ugly requirement w.r.t. shib is that it depends on exact URL locations (these are proxied from nginx -> apache -> mod_shib), so this dispatcher will have to use redirects instead of hooking up appropriate backend functions, at least for shib.

This comment has been minimized.

@jbau

jbau Jun 5, 2013

Hmm. A bug in github? The discussion pane doesn't show the updated diff (i.e. it still has "shib" hardcoded here), but the files pane looks right.

@jbau

jbau Jun 5, 2013

Hmm. A bug in github? The discussion pane doesn't show the updated diff (i.e. it still has "shib" hardcoded here), but the files pane looks right.

@@ -679,6 +683,8 @@ def create_account(request, post_override=None):
login_user.is_active = True
login_user.save()
+ try_change_enrollment(request)

This comment has been minimized.

@jbau

jbau Jun 6, 2013

need this to be after the eamap.user = login_user, since the enrollment authorization check will need an updated eamap

@jbau

jbau Jun 6, 2013

need this to be after the eamap.user = login_user, since the enrollment authorization check will need an updated eamap

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 11, 2013

Added a bunch of tests that all pass (run with rake test). So, removing the DO NOT MERGE label.

@cpennington do you have time for another look? possibly @chrisndodge @ormsbee or @shnayder ?

jbau commented Jun 11, 2013

Added a bunch of tests that all pass (run with rake test). So, removing the DO NOT MERGE label.

@cpennington do you have time for another look? possibly @chrisndodge @ormsbee or @shnayder ?

+
+ Uses django test client for its session support
+ """
+ if not settings.MITX_FEATURES.get('AUTH_USE_SHIB'):

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Please use @unittest.skipUnless so that it's explicitly marked when these tests are skipped.

@ormsbee

ormsbee Jun 11, 2013

Member

Please use @unittest.skipUnless so that it's explicitly marked when these tests are skipped.

This comment has been minimized.

@jbau

jbau Jun 12, 2013

yup. thanks for the suggestion, will change.

@jbau

jbau Jun 12, 2013

yup. thanks for the suggestion, will change.

+ 'REMOTE_USER': REMOTE_USER})
+ request.user = AnonymousUser()
+ response = shib_login(request)
+ if idp is "https://idp.stanford.edu" and REMOTE_USER is 'testuser@stanford.edu':

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Don't use "is" for string comparisons (it compares object identity, and isn't guaranteed to work).

@ormsbee

ormsbee Jun 11, 2013

Member

Don't use "is" for string comparisons (it compares object identity, and isn't guaranteed to work).

This comment has been minimized.

@jbau

jbau Jun 12, 2013

ah, good catch.

@jbau

jbau Jun 12, 2013

ah, good catch.

+ try:
+ self.store = modulestore('direct')
+ except KeyError:
+ self.store = modulestore()

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Comment what's going on here?

@ormsbee

ormsbee Jun 11, 2013

Member

Comment what's going on here?

This comment has been minimized.

@jbau

jbau Jun 12, 2013

cribbed from https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/modulestore/tests/factories.py#L31
but it seems like just setting to modulestore() work fine, so will go with that.

@jbau

jbau Jun 12, 2013

cribbed from https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/modulestore/tests/factories.py#L31
but it seems like just setting to modulestore() work fine, so will go with that.

+ Uses django test client for its session support
+ """
+ if not settings.MITX_FEATURES.get('AUTH_USE_SHIB'):
+ return

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Please use explicit skip

@ormsbee

ormsbee Jun 11, 2013

Member

Please use explicit skip

+ except KeyError:
+ self.store = modulestore()
+
+ def __test_looper(self, testfn):

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Is there a particular reason you're using a double-underscored method here?

@ormsbee

ormsbee Jun 11, 2013

Member

Is there a particular reason you're using a double-underscored method here?

This comment has been minimized.

@jbau

jbau Jun 12, 2013

Nope. will change.

@jbau

jbau Jun 12, 2013

Nope. will change.

+ meta_dict.update({'givenName': givenName})
+ if sn is not None:
+ meta_dict.update({'sn': sn})
+ testfn(meta_dict, mail, givenName, sn)

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Instead of having a function passed into this loop, it would probably be clearer to make this into a generator. Since it's referencing all module level vars anyway you could do something like:

def all_test_identities():
    for mail in mails:
        for givenName in givenNames:
            for sn in sns:
                yield build_identity_dict(REMOTE_USER, mail, givenName, sn)

# Then you can use it in your other tests like:

def _test_foo(self):
    for identity in all_test_identities():
        self.testSomething(etc)
@ormsbee

ormsbee Jun 11, 2013

Member

Instead of having a function passed into this loop, it would probably be clearer to make this into a generator. Since it's referencing all module level vars anyway you could do something like:

def all_test_identities():
    for mail in mails:
        for givenName in givenNames:
            for sn in sns:
                yield build_identity_dict(REMOTE_USER, mail, givenName, sn)

# Then you can use it in your other tests like:

def _test_foo(self):
    for identity in all_test_identities():
        self.testSomething(etc)

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Or it could yield a namedtuple -- whatever's most convenient for use in your testing methods.

@ormsbee

ormsbee Jun 11, 2013

Member

Or it could yield a namedtuple -- whatever's most convenient for use in your testing methods.

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

You could also strip the mail, givenName, sn here so you don't have to do those (foo is None) or (foo.strip() == "") checks later.

@ormsbee

ormsbee Jun 11, 2013

Member

You could also strip the mail, givenName, sn here so you don't have to do those (foo is None) or (foo.strip() == "") checks later.

This comment has been minimized.

@jbau

jbau Jun 12, 2013

Thanks, the generator is a great suggestion and I'll be happy to switch to it.

@jbau

jbau Jun 12, 2013

Thanks, the generator is a great suggestion and I'll be happy to switch to it.

+mails = [None, '', 'test_user@stanford.edu']
+givenNames = [None, '', 'Jason', 'jason; John; bob'] # At Stanford, the givenNames can be a list delimited by ';'
+sns = [None, '', 'Bau', 'bau; smith'] # At Stanford, the sns can be a list delimited by ';'
+

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Please use Python conventions for your variables. All caps for constants, underscores for var names.

@ormsbee

ormsbee Jun 11, 2013

Member

Please use Python conventions for your variables. All caps for constants, underscores for var names.

This comment has been minimized.

@jbau

jbau Jun 12, 2013

yeah, I will convert these. I matched the case of what comes out of Shib into request.META, but obviously that's not very pythonic.

@jbau

jbau Jun 12, 2013

yeah, I will convert these. I matched the case of what comes out of Shib into request.META, but obviously that's not very pythonic.

+ shib-login is called, while a user without such gets the registration form.
+ """
+ if not settings.MITX_FEATURES.get('AUTH_USE_SHIB'):
+ return

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Please use explicit skipping of test.

@ormsbee

ormsbee Jun 11, 2013

Member

Please use explicit skipping of test.

+ idps = ['https://idp.stanford.edu/', 'https://someother.idp.com/']
+ REMOTE_USERS = ['testuser@stanford.edu', 'testuser2@someother_idp.com']
+
+ for idp in idps:

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

It's a little confusing here since you're shadowing the module level idp (though I guess if you change that one to IDP, this issue goes away).

@ormsbee

ormsbee Jun 11, 2013

Member

It's a little confusing here since you're shadowing the module level idp (though I guess if you change that one to IDP, this issue goes away).

+ request.user = AnonymousUser()
+ response = shib_login(request)
+ if idp is "https://idp.stanford.edu" and REMOTE_USER is 'testuser@stanford.edu':
+ self.assertTrue(isinstance(response, HttpResponseRedirect))

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

There's a self.assertIsInstance()

@ormsbee

ormsbee Jun 11, 2013

Member

There's a self.assertIsInstance()

This comment has been minimized.

@jbau

jbau Jun 12, 2013

will switch

@jbau

jbau Jun 12, 2013

will switch

+ factory = RequestFactory()
+
+ # Putting this in setUp instead of __init__ due to interactions with @override_settings that
+ # I don't want to debug right now

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

It should be in setUp() anyway.

@ormsbee

ormsbee Jun 11, 2013

Member

It should be in setUp() anyway.

+ return
+
+ def _test_helper(meta_dict, mail, givenName, sn):
+ self.client.logout()

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Please replace _test_helper() entries by using the generator described above.

@ormsbee

ormsbee Jun 11, 2013

Member

Please replace _test_helper() entries by using the generator described above.

+ 'REMOTE_USER': REMOTE_USER})
+ if mail is not None:
+ meta_dict.update({'mail': mail})
+ if givenName is not None:

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Is there a reason to prefer dynamically adding keys, as opposed to always having the same keys and just having a value of None when there's nothing there?

@ormsbee

ormsbee Jun 11, 2013

Member

Is there a reason to prefer dynamically adding keys, as opposed to always having the same keys and just having a value of None when there's nothing there?

This comment has been minimized.

@jbau

jbau Jun 12, 2013

There is in fact a reason for this one. I've found that setting, say, mail to None creates requests with META querydicts having a key of mail and a value of None.

So, for instance, request.META.get('mail', '') would return None instead of ''. Hence the change to dynamically adding keys, which more directly mimics server behavior in any case.

@jbau

jbau Jun 12, 2013

There is in fact a reason for this one. I've found that setting, say, mail to None creates requests with META querydicts having a key of mail and a value of None.

So, for instance, request.META.get('mail', '') would return None instead of ''. Hence the change to dynamically adding keys, which more directly mimics server behavior in any case.

This comment has been minimized.

@jbau

jbau Jun 12, 2013

i.e. I wanted None to denote a case where the key was truly absent.

@jbau

jbau Jun 12, 2013

i.e. I wanted None to denote a case where the key was truly absent.

+from student.tests.factories import UserFactory
+from external_auth.models import ExternalAuthMap
+from student.views import create_account, change_enrollment
+from student.models import UserProfile, Registration, CourseEnrollment

This comment has been minimized.

@ormsbee

ormsbee Jun 11, 2013

Member

Please group the imports. Stdlib first (I guess none of those here?), then Django, then xmodule, then our own Django app stuff.

@ormsbee

ormsbee Jun 11, 2013

Member

Please group the imports. Stdlib first (I guess none of those here?), then Django, then xmodule, then our own Django app stuff.

This comment has been minimized.

@jbau

jbau Jun 12, 2013

yup. will do.

@jbau

jbau Jun 12, 2013

yup. will do.

@ormsbee

This comment has been minimized.

Show comment
Hide comment
@ormsbee

ormsbee Jun 11, 2013

Member

Sorry, not done yet, but I have to go. Will pick it up tomorrow morning.

Member

ormsbee commented Jun 11, 2013

Sorry, not done yet, but I have to go. Will pick it up tomorrow morning.

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 12, 2013

thanks for all the comments @ormsbee . I know you're not done yet, but let me start addressing some

jbau commented Jun 12, 2013

thanks for all the comments @ormsbee . I know you're not done yet, but let me start addressing some

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 12, 2013

Most of the outdated diff discussions above have my text replies. Also new code, naturally.

jbau commented Jun 12, 2013

Most of the outdated diff discussions above have my text replies. Also new code, naturally.

+
+
+def gen_all_identities():
+ """A generator for all combinations of identity inputs"""

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

My understanding is that this is generating HTTP request headers that will be put into request.META for each relevant identity. Could you please specify in the docstring exactly what the dict represents?

@ormsbee

ormsbee Jun 13, 2013

Member

My understanding is that this is generating HTTP request headers that will be put into request.META for each relevant identity. Could you please specify in the docstring exactly what the dict represents?

This comment has been minimized.

@jbau

jbau Jun 18, 2013

will do.

+ if given_name is not None:
+ meta_dict.update({'givenName': given_name})
+ if surname is not None:
+ meta_dict.update({'sn': surname})

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Very minor point, but you don't need to use update(), you could just do:

meta_dict = {'Shib-Identity-Provider': IDP,
             'REMOTE_USER': REMOTE_USER}
if mail is not None:
    meta_dict['mail'] = mail
if given_name is not None:
    meta_dict['givenName'] = given_name
if surname is not None:
    meta_dict['sn'] = surname
@ormsbee

ormsbee Jun 13, 2013

Member

Very minor point, but you don't need to use update(), you could just do:

meta_dict = {'Shib-Identity-Provider': IDP,
             'REMOTE_USER': REMOTE_USER}
if mail is not None:
    meta_dict['mail'] = mail
if given_name is not None:
    meta_dict['givenName'] = given_name
if surname is not None:
    meta_dict['sn'] = surname

This comment has been minimized.

@jbau

jbau Jun 18, 2013

will do also

@jbau

jbau Jun 18, 2013

will do also

+ Tests for the Shibboleth SP, which communicates via request.META
+ (Apache environment variables set by mod_shib)
+ """
+ factory = RequestFactory()

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Please name this more descriptively.

@ormsbee

ormsbee Jun 13, 2013

Member

Please name this more descriptively.

This comment has been minimized.

@jbau

jbau Jun 18, 2013

changed to request_factory

@jbau

jbau Jun 18, 2013

changed to request_factory

+ self.assertEqual(response['Location'], '/')
+ else:
+ self.assertEqual(response.status_code, 200)
+ self.assertContains(response, "<title>Register for")

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

So in general, I'm a bit leery of having if conditionals like this in test code. For instance, if someone for some reason changed the remote user testuser@stanford.edu above but not here, then this test wouldn't fail -- it would just never actually execute some of the asserts. It's pretty local here, so I'm not going to be adamant about it, but just a concern.

@ormsbee

ormsbee Jun 13, 2013

Member

So in general, I'm a bit leery of having if conditionals like this in test code. For instance, if someone for some reason changed the remote user testuser@stanford.edu above but not here, then this test wouldn't fail -- it would just never actually execute some of the asserts. It's pretty local here, so I'm not going to be adamant about it, but just a concern.

+ Uses django test client for its session support
+ """
+ for identity in gen_all_identities():
+ self.client.logout()

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Instead of doing logout() and the cleanup, can we just make a new Client() object every time through the loop?

@ormsbee

ormsbee Jun 13, 2013

Member

Instead of doing logout() and the cleanup, can we just make a new Client() object every time through the loop?

This comment has been minimized.

+ self.client.logout()
+ request_kwargs = {'path': '/shib-login/', 'data': {}, 'follow': False}
+ request_kwargs.update(identity)
+ response = self.client.get(**request_kwargs) # identity k/v pairs will show up in request.META

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

You could also say:

request_kwargs = dict(identity, path='/shib-login/', data={}, follow=False)

Or even just:

response = self.client.get(path='/shib-login/', data={}, follow=False, **identity)
@ormsbee

ormsbee Jun 13, 2013

Member

You could also say:

request_kwargs = dict(identity, path='/shib-login/', data={}, follow=False)

Or even just:

response = self.client.get(path='/shib-login/', data={}, follow=False, **identity)

This comment has been minimized.

@jbau

jbau Jun 19, 2013

changed

+ else:
+ self.assertNotContains(response, mail_input_HTML)
+ sn_empty = identity.get('sn', '') == ''
+ given_name_empty = identity.get('givenName', '') == ''

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Trivial change, but you could say sn_empty = not identity.get('sn') if you wanted

@ormsbee

ormsbee Jun 13, 2013

Member

Trivial change, but you could say sn_empty = not identity.get('sn') if you wanted

This comment has been minimized.

@jbau

jbau Jun 19, 2013

changed

+ student3 = UserFactory.create()
+ student3.username = "teststudent3"
+ student3.email = "teststudent3@gmail.com"
+ student3.save()

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Could you please make the vars more descriptive (open_course, limited_enrollment_course, etc.) Or would it make more sense to make separate tests out of these?

@ormsbee

ormsbee Jun 13, 2013

Member

Could you please make the vars more descriptive (open_course, limited_enrollment_course, etc.) Or would it make more sense to make separate tests out of these?

This comment has been minimized.

@jbau

jbau Jun 19, 2013

yes, this is done.

shib_course, open_enroll_course

shib_student, other_ext_student, int_student

@jbau

jbau Jun 19, 2013

yes, this is done.

shib_course, open_enroll_course

shib_student, other_ext_student, int_student

+ CourseEnrollment.objects.filter(user=student, course_id=course.id).delete()
+ else:
+ self.assertEqual(response.status_code, 400)
+ self.assertEqual(CourseEnrollment.objects.filter(user=student, course_id=course.id).count(), 0)

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Again, my if paranoia.

@ormsbee

ormsbee Jun 13, 2013

Member

Again, my if paranoia.

This comment has been minimized.

@jbau

jbau Jun 19, 2013

changed to ==

@jbau

jbau Jun 19, 2013

changed to ==

This comment has been minimized.

@jbau

jbau Jun 19, 2013

actually, i misunderstood what you mean here. I guess the argument is that since there's an unconditional else clause, at least some set of asserts will always execute, and the fact that those pass should indicate a good passing test.

@jbau

jbau Jun 19, 2013

actually, i misunderstood what you mean here. I guess the argument is that since there's an unconditional else clause, at least some set of asserts will always execute, and the fact that those pass should indicate a good passing test.

common/djangoapps/external_auth/views.py
+ # Now to try enrollment
+ # Need to special case Shibboleth here because it logs in via a GET.
+ # testing request.method for extra paranoia
+ if 'shib:' in external_domain and request.method == 'GET':

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

No feature flag toggle necessary?

@ormsbee

ormsbee Jun 13, 2013

Member

No feature flag toggle necessary?

This comment has been minimized.

common/djangoapps/external_auth/views.py
+ shib = {}
+
+ for attr in attrs:
+ shib[attr] = request.META.get(attr, '')

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Very minor language feature, you can do:

shib = {k: request.META.get(k, '')
        for k in ['REMOTE_USER', 'givenName', 'sn', 'mail', 'Shib-Identity-Provider']}
@ormsbee

ormsbee Jun 13, 2013

Member

Very minor language feature, you can do:

shib = {k: request.META.get(k, '')
        for k in ['REMOTE_USER', 'givenName', 'sn', 'mail', 'Shib-Identity-Provider']}

This comment has been minimized.

common/djangoapps/external_auth/views.py
+ external_domain="shib:" + shib['Shib-Identity-Provider'],
+ credentials=shib,
+ email=shib['mail'],
+ fullname="%s %s" % (shib['givenName'], shib['sn']),

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Has this been tested for non-ascii names?

@ormsbee

ormsbee Jun 13, 2013

Member

Has this been tested for non-ascii names?

This comment has been minimized.

@jbau

jbau Jun 19, 2013

adding 包 to one of the test surnames

@jbau

jbau Jun 19, 2013

adding 包 to one of the test surnames

+ This in turn typically uses EduPersonPrincipalName
+ http://www.incommonfederation.org/attributesummary.html#eduPersonPrincipal
+ but the configuration is in the shibboleth software.
+ """

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

Please describe your input args, particularly what is expected of retfun

@ormsbee

ormsbee Jun 13, 2013

Member

Please describe your input args, particularly what is expected of retfun

This comment has been minimized.

@jbau

jbau Jun 19, 2013

I just went ahead and removed retfun as an arg

@jbau

jbau Jun 19, 2013

I just went ahead and removed retfun as an arg

+ if "course_id" not in enroll_request.POST and "course_id" in enroll_request.GET:
+ enroll_request.POST.setdefault('course_id', enroll_request.GET.get('course_id'))
+
+ return enroll_request

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

How does this interact with Django's csrf handling?

@ormsbee

ormsbee Jun 13, 2013

Member

How does this interact with Django's csrf handling?

This comment has been minimized.

@jbau

jbau Jun 19, 2013

It seems to work without setting request.COOKIE['CSRFTOKEN']. I think because we are creating a mock request that doesn't go through django middleware, so the presence of that cookie is not checked (since that check's in CSRF middleware).

Actual CSRF protection (nonces in cookies) here is handled by shibboleth. (The url to cause login is utterly predictable ('/shib-login/', but there is a series of POSTs and redirects handled by apache/mod_shib)

@jbau

jbau Jun 19, 2013

It seems to work without setting request.COOKIE['CSRFTOKEN']. I think because we are creating a mock request that doesn't go through django middleware, so the presence of that cookie is not checked (since that check's in CSRF middleware).

Actual CSRF protection (nonces in cookies) here is handled by shibboleth. (The url to cause login is utterly predictable ('/shib-login/', but there is a series of POSTs and redirects handled by apache/mod_shib)

@@ -138,6 +138,10 @@
#Timezone overrides
TIME_ZONE = ENV_TOKENS.get('TIME_ZONE', TIME_ZONE)
+#Additional installed apps
+for app in ENV_TOKENS.get('ADDL_INSTALLED_APPS', []):
+ INSTALLED_APPS += (app,)

This comment has been minimized.

@ormsbee

ormsbee Jun 13, 2013

Member

This dynamic app list is probably overkill. @cpennington: as long as the feature flag is disabled, do you see any harm in just always having the app in?

@ormsbee

ormsbee Jun 13, 2013

Member

This dynamic app list is probably overkill. @cpennington: as long as the feature flag is disabled, do you see any harm in just always having the app in?

This comment has been minimized.

@ormsbee

This comment has been minimized.

Show comment
Hide comment
@ormsbee

ormsbee Jun 13, 2013

Member

Aside from the comments I made, I would also appreciate seeing some logging of shibboleth activity -- nothing sensitive of course, just so that we can detect if shib in particular is failing for some reason.

Thank you.

Member

ormsbee commented Jun 13, 2013

Aside from the comments I made, I would also appreciate seeing some logging of shibboleth activity -- nothing sensitive of course, just so that we can detect if shib in particular is failing for some reason.

Thank you.

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 18, 2013

had lots of distractions (and cherry-picked this into our release), but finally now circling back to finish this off.

jbau commented Jun 18, 2013

had lots of distractions (and cherry-picked this into our release), but finally now circling back to finish this off.

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 19, 2013

OK. Addressed most of @ormsbee 's comments and repushed. Added a few more tests and a bunch more loggin. Jenkins tests passing.
I did leave the dynamic app list in, just b/c I thought that'd be a useful hook (and to save you guys from having to run a migration whenever this is deployed).

I'm ready to merge this, but one last round of comments welcome @ormsbee and @cpennington

jbau commented Jun 19, 2013

OK. Addressed most of @ormsbee 's comments and repushed. Added a few more tests and a bunch more loggin. Jenkins tests passing.
I did leave the dynamic app list in, just b/c I thought that'd be a useful hook (and to save you guys from having to run a migration whenever this is deployed).

I'm ready to merge this, but one last round of comments welcome @ormsbee and @cpennington

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 19, 2013

oh, i should probably squash commits I'll leave the last 2 individual b/c that's what will make it easiest to merge into edx-west's release

jbau commented Jun 19, 2013

oh, i should probably squash commits I'll leave the last 2 individual b/c that's what will make it easiest to merge into edx-west's release

Jason Bau added some commits Jun 4, 2013

Jason Bau
AWS settings hook for addl installed apps + lms/wsgi_apache.py
wsgi_apache.py passes an apache env variable for SERVICE_VARIANT
to the os environment, where it's normally set when we use gunicorn
Jason Bau
The bulk of Shibboleth authentication for Stanford
Highlights:
* The url '/shib-login/' interfaces with apache/mod_shib via
  request.META to handle shibboleth login and registrations
* Courses can designate 'enrollment_domains' to limit enrollment
  to users with a linked ExternalAuthMap verified by a particular
  identity provider
* Tests
* Logging

 Changes to be committed:

	new file:   common/djangoapps/external_auth/migrations/0001_initial.py
	new file:   common/djangoapps/external_auth/migrations/__init__.py
	new file:   common/djangoapps/external_auth/tests/test_shib.py
	modified:   common/djangoapps/external_auth/views.py
	modified:   common/djangoapps/student/views.py
	modified:   common/lib/xmodule/xmodule/course_module.py
	modified:   lms/djangoapps/courseware/access.py
	modified:   lms/djangoapps/courseware/tests/test_access.py
	modified:   lms/envs/common.py
	modified:   lms/envs/dev.py
	modified:   lms/envs/test.py
	modified:   lms/templates/courseware/course_about.html
	modified:   lms/templates/dashboard.html
	modified:   lms/templates/extauth_failure.html
	modified:   lms/templates/navigation.html
	modified:   lms/templates/register.html
	modified:   lms/templates/signup_modal.html
	modified:   lms/urls.py
	renamed:    lms/wsgi_apache.py -> lms/wsgi_apache_lms.py
Jason Bau
Turn off Agreement to Terms of Service for Stanford shib
As stipulated by Stanford's office of general counsel
common/djangoapps/external_auth/views.py
+ """))
+
+ if not request.META.get('REMOTE_USER'):
+ log.exception("SHIB: no REMOTE_USER found in request.META")

This comment has been minimized.

@ormsbee

ormsbee Jun 20, 2013

Member

Please use log.error() not log.exception(), since there is no exception here. Also, if I saw this message come out in the log, would there be any way to tie it to a user (so we could try to reproduce the error, or see what org they belong to)?

@ormsbee

ormsbee Jun 20, 2013

Member

Please use log.error() not log.exception(), since there is no exception here. Also, if I saw this message come out in the log, would there be any way to tie it to a user (so we could try to reproduce the error, or see what org they belong to)?

common/djangoapps/external_auth/views.py
+ log.exception("SHIB: no REMOTE_USER found in request.META")
+ return default_render_failure(request, shib_error_msg)
+ elif not request.META.get('Shib-Identity-Provider'):
+ log.exception("SHIB: no Shib-Identity-Provider in request.META")

This comment has been minimized.

@ormsbee

ormsbee Jun 20, 2013

Member

Same notes as just above on logging.

@ormsbee

ormsbee Jun 20, 2013

Member

Same notes as just above on logging.

@ormsbee

This comment has been minimized.

Show comment
Hide comment
@ormsbee

ormsbee Jun 20, 2013

Member

Except for the minor note/question on logging, this works for me. @cpennington ?

Member

ormsbee commented Jun 20, 2013

Except for the minor note/question on logging, this works for me. @cpennington ?

+REMOTE_USER = 'test_user@stanford.edu'
+MAILS = [None, '', 'test_user@stanford.edu']
+GIVENNAMES = [None, '', 'Jason', 'jasön; John; bob'] # At Stanford, the givenNames can be a list delimited by ';'
+SNS = [None, '', 'Bau', '包; smith'] # At Stanford, the sns can be a list delimited by ';'

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

I think this would be easier to maintain if we used as ascii escape sequence here, rather than a utf-8 encoded file.

@cpennington

cpennington Jun 20, 2013

Member

I think this would be easier to maintain if we used as ascii escape sequence here, rather than a utf-8 encoded file.

+ def setUp(self):
+ self.store = modulestore()
+
+ @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), True)

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

Why skip these tests, rather than just setting AUTH_USE_SHIB for the purpose of the tests? As written, these tests only get run sometimes, which leads to the possibility that they'll get inadvertently broken and no one will notice for a while.

@cpennington

cpennington Jun 20, 2013

Member

Why skip these tests, rather than just setting AUTH_USE_SHIB for the purpose of the tests? As written, these tests only get run sometimes, which leads to the possibility that they'll get inadvertently broken and no one will notice for a while.

+ failure_msg = _(dedent("""
+ You have already created an account using an external login like WebAuth or Shibboleth.
+ Please contact %s for support """
+ % getattr(settings, 'TECH_SUPPORT_EMAIL', 'techsupport@class.stanford.edu')))

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

Indentation is pretty wonky here. (Minor nitpick, I know)

@cpennington

cpennington Jun 20, 2013

Member

Indentation is pretty wonky here. (Minor nitpick, I know)

-
- uname = internal_user.username
- user = authenticate(username=uname, password=eamap.internal_password)
+ if settings.MITX_FEATURES.get('AUTH_USE_SHIB'):

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

It seems like this logic of how to map the external auth to the internal user is going to vary over most types of external auth. Can we push this down into an external auth provider class, so that as we have more types of providers, they can each have their own logic, and we don't have to have a bunch of conditionals in this chunk of the code?

@cpennington

cpennington Jun 20, 2013

Member

It seems like this logic of how to map the external auth to the internal user is going to vary over most types of external auth. Can we push this down into an external auth provider class, so that as we have more types of providers, they can each have their own logic, and we don't have to have a bunch of conditionals in this chunk of the code?

+ return signup(request, eamap)
+
+ # We trust shib's authentication, so no need to authenticate using the password again
+ if settings.MITX_FEATURES.get('AUTH_USE_SHIB'):

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

Again, it would be nice to leave this up to the external auth provider.

@cpennington

cpennington Jun 20, 2013

Member

Again, it would be nice to leave this up to the external auth provider.

common/djangoapps/external_auth/views.py
- log.debug('Doing signup for %s' % eamap.external_email)
+ # Can't have terms of service for Stanford users, according to Stanford's Office of General Counsel
+ if settings.MITX_FEATURES['AUTH_USE_SHIB'] and ('stanford' in eamap.external_domain):
+ context['ask_for_tos'] = False

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

Probably necessary in the short term, but ick.

@cpennington

cpennington Jun 20, 2013

Member

Probably necessary in the short term, but ick.

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

I would much rather have this as something that is set as part of environment settings than to have stanford hardcoded in.

@cpennington

cpennington Jun 20, 2013

Member

I would much rather have this as something that is set as part of environment settings than to have stanford hardcoded in.

common/djangoapps/student/views.py
+
+ # get info w.r.t ExternalAuthMap
+ external_auth_map = None
+ if 'external_auth' in settings.INSTALLED_APPS:

This comment has been minimized.

@cpennington

cpennington Jun 20, 2013

Member

Honestly, I think I'd rather just have external auth always in INSTALLED_APPS, especially since there are separate feature flags for each kind of auth provider (although it might be nice to have that just be a list of auth providers instead of single flags).

@cpennington

cpennington Jun 20, 2013

Member

Honestly, I think I'd rather just have external auth always in INSTALLED_APPS, especially since there are separate feature flags for each kind of auth provider (although it might be nice to have that just be a list of auth providers instead of single flags).

@cpennington

This comment has been minimized.

Show comment
Hide comment
@cpennington

cpennington Jun 20, 2013

Member

This is probably broader than this particular PR, but it seems like we should be able to come up with a way to make external auth providers pluggable. It seems like they should be django apps, since they each have their own views, and then they should just take advantage of the core external auth functionality. It also seems like there might be a django app out there that already does a lot of this.

Member

cpennington commented Jun 20, 2013

This is probably broader than this particular PR, but it seems like we should be able to come up with a way to make external auth providers pluggable. It seems like they should be django apps, since they each have their own views, and then they should just take advantage of the core external auth functionality. It also seems like there might be a django app out there that already does a lot of this.

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 20, 2013

@cpennington , thanks for the review. As you observed, this PR is honestly tailored to getting Stanford Shib up and running in a short amount of time. There is indeed a need to generalize external auth, and it's fairly high on my priority list to get working on that front.

Can we negotiate a bit on this PR? Let's consider this a "patch" for Stanford Shib that shouldn't break other existing installations of external auth (my guess is MIT SSL), with generalizing Auth in mind as a prioritized follow on. This code is basically released on our instance, and I'd hate for it to start drifting from master.

With that in mind, I can fix the following:

  • log.exception -> log.error
  • '包' -> '\xe5\x8c\x85'
  • skipping tests -> changing test settings
  • 'external_auth' always in installed_apps
  • finding some way to get hardcoded 'stanford' out of codebase, probably in a setting.

and then I'm planning to merge this, with generalization to come in follow on work.

Sound good, @cpennington ?

jbau commented Jun 20, 2013

@cpennington , thanks for the review. As you observed, this PR is honestly tailored to getting Stanford Shib up and running in a short amount of time. There is indeed a need to generalize external auth, and it's fairly high on my priority list to get working on that front.

Can we negotiate a bit on this PR? Let's consider this a "patch" for Stanford Shib that shouldn't break other existing installations of external auth (my guess is MIT SSL), with generalizing Auth in mind as a prioritized follow on. This code is basically released on our instance, and I'd hate for it to start drifting from master.

With that in mind, I can fix the following:

  • log.exception -> log.error
  • '包' -> '\xe5\x8c\x85'
  • skipping tests -> changing test settings
  • 'external_auth' always in installed_apps
  • finding some way to get hardcoded 'stanford' out of codebase, probably in a setting.

and then I'm planning to merge this, with generalization to come in follow on work.

Sound good, @cpennington ?

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 20, 2013

@ormsbee @cpennington
As I think more about it, the real issue with why I was skipping tests when AUTH_USE_SHIB was not set. Mainly, external_auth, even though it is under common, is really only designed to work with LMS. There is existing evidence for this, for example the lack of openid provider or consumer URLs in cms/urls.py.

Since this is the case, I think it's better to skip these shib tests when AUTH_USE_SHIB is not set (like when testing cms), rather than artificially adding things like shib urls to cms/urls.py that won't be used anyway, just to make tests pass. Note that I have added to lms.envs.tests.py, so these test do run on every test run, just in the lms context and not in the cms context.

Finally, there's also already precedence for this sort of skipping. See:
https://github.com/edx/edx-platform/blob/master/common/djangoapps/external_auth/tests/test_openid_provider.py#L75 and
https://github.com/edx/edx-platform/blob/master/common/djangoapps/external_auth/tests/test_openid_provider.py#L104

jbau commented Jun 20, 2013

@ormsbee @cpennington
As I think more about it, the real issue with why I was skipping tests when AUTH_USE_SHIB was not set. Mainly, external_auth, even though it is under common, is really only designed to work with LMS. There is existing evidence for this, for example the lack of openid provider or consumer URLs in cms/urls.py.

Since this is the case, I think it's better to skip these shib tests when AUTH_USE_SHIB is not set (like when testing cms), rather than artificially adding things like shib urls to cms/urls.py that won't be used anyway, just to make tests pass. Note that I have added to lms.envs.tests.py, so these test do run on every test run, just in the lms context and not in the cms context.

Finally, there's also already precedence for this sort of skipping. See:
https://github.com/edx/edx-platform/blob/master/common/djangoapps/external_auth/tests/test_openid_provider.py#L75 and
https://github.com/edx/edx-platform/blob/master/common/djangoapps/external_auth/tests/test_openid_provider.py#L104

Jason Bau
Shib PR responses to @cpennington and @ormsbee comments
* Changed unicode test cases to ascii encoding
* Removed 'stanford' hardcoding in TOS logic in lieu of
  'SHIB_DISABLE_TOS' MIT_FEATURES flag
* made 'external_auth' always an installed_app in lms
* log.exception changd to log.error where appropriate

But: did not change skipping tests to changing settings, for
reasons stated here:
#67 (comment)
@cpennington

This comment has been minimized.

Show comment
Hide comment
@cpennington

cpennington Jun 21, 2013

Member

Your reasons make sense for this pull request. Longer term, I think we actually want the same authentication available both in studio and the LMS. To make that easier, we should move the urls into a urls.py inside the django app, rather than outside, and just include the urls using django's include mechanism. Until then, though, it makes sense to skip the tests when in the CMS.

Member

cpennington commented Jun 21, 2013

Your reasons make sense for this pull request. Longer term, I think we actually want the same authentication available both in studio and the LMS. To make that easier, we should move the urls into a urls.py inside the django app, rather than outside, and just include the urls using django's include mechanism. Until then, though, it makes sense to skip the tests when in the CMS.

@jbau

This comment has been minimized.

Show comment
Hide comment
@jbau

jbau Jun 21, 2013

ok tests are passing. I am planning to merge this tomorrow morning PDT, but last minute blocker comments are fair game until then.

jbau commented Jun 21, 2013

ok tests are passing. I am planning to merge this tomorrow morning PDT, but last minute blocker comments are fair game until then.

jbau pushed a commit that referenced this pull request Jun 21, 2013

@jbau jbau merged commit 07e56ac into master Jun 21, 2013

1 check passed

default Build #8270 passed
Details

chrisrossi referenced this pull request in jazkarta/edx-platform Mar 31, 2014

aboudreault pushed a commit to aboudreault/edx-platform that referenced this pull request Jun 4, 2014

Merge pull request #67 from edx-solutions/muhhshoaib/adding_start_and…
…_end_date_fields_to_course_objects_1302

Muhhshoaib/adding start and end date fields to course objects 1302

jbau pushed a commit that referenced this pull request Oct 10, 2014

naeem91 pushed a commit to naeem91/edx-platform that referenced this pull request Feb 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment