From 10ebcefd5427a8083581a877990517d0485ee5b1 Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 1 Aug 2012 14:59:55 -0400 Subject: [PATCH 01/22] add openid authentication --- common/djangoapps/student/views.py | 45 +++++- lms/envs/dev.py | 12 ++ lms/envs/dev_ike.py | 222 ++++++++++++----------------- lms/templates/extauth_failure.html | 11 ++ lms/templates/index.html | 9 ++ lms/templates/login_modal.html | 3 + lms/templates/signup_modal.html | 15 ++ lms/urls.py | 12 +- requirements.txt | 2 + 9 files changed, 199 insertions(+), 132 deletions(-) create mode 100644 lms/templates/extauth_failure.html diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index b739e0e37c..692b135ff6 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -61,6 +61,15 @@ def index(request): if settings.COURSEWARE_ENABLED and request.user.is_authenticated(): return redirect(reverse('dashboard')) + return main_index() + +def main_index(extra_context = {}): + ''' + Render the edX main page. + + extra_context is used to allow immediate display of certain modal windows, eg signup, + as used by external_auth. + ''' feed_data = cache.get("students_index_rss_feed_data") if feed_data == None: if hasattr(settings, 'RSS_URL'): @@ -81,8 +90,9 @@ def index(request): for course in courses: universities[course.org].append(course) - return render_to_response('index.html', {'universities': universities, 'entries': entries}) - + context = {'universities': universities, 'entries': entries} + context.update(extra_context) + return render_to_response('index.html', context) def course_from_id(id): course_loc = CourseDescriptor.id_to_location(id) @@ -257,11 +267,26 @@ def change_setting(request): @ensure_csrf_cookie def create_account(request, post_override=None): - ''' JSON call to enroll in the course. ''' + ''' + JSON call to create new edX account. + Used by form in signup_modal.html, which is included into navigation.html + ''' js = {'success': False} post_vars = post_override if post_override else request.POST + # if doing signup for an external authorization, then get email, password, name from the eamap + # don't use the ones from the form, since the user could have hacked those + doExternalAuth = 'ExternalAuthMap' in request.session + if doExternalAuth: + eamap = request.session['ExternalAuthMap'] + email = eamap.external_email + name = eamap.external_name + password = eamap.internal_password + post_vars = dict(post_vars.items()) + post_vars.update(dict(email=email, name=name, password=password, username=post_vars['username'])) + log.debug('extauth test: post_vars = %s' % post_vars) + # Confirm we have a properly formed request for a in ['username', 'email', 'password', 'name']: if a not in post_vars: @@ -356,8 +381,9 @@ def create_account(request, post_override=None): 'key': r.activation_key, } + # composes activation email subject = render_to_string('emails/activation_email_subject.txt', d) - # Email subject *must not* contain newlines + # Email subject *must not* contain newlines subject = ''.join(subject.splitlines()) message = render_to_string('emails/activation_email.txt', d) @@ -382,6 +408,17 @@ def create_account(request, post_override=None): try_change_enrollment(request) + if doExternalAuth: + eamap.user = login_user + eamap.dtsignup = datetime.datetime.now() + eamap.save() + log.debug('Updated ExternalAuthMap for %s to be %s' % (post_vars['username'],eamap)) + + if settings.MITX_FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'): + log.debug('bypassing activation email') + login_user.is_active = True + login_user.save() + js = {'success': True} return HttpResponse(json.dumps(js), mimetype="application/json") diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 1a2659cb1f..f9b7ba10a0 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -55,6 +55,18 @@ CACHES = { # Dummy secret key for dev SECRET_KEY = '85920908f28904ed733fe576320db18cabd7b6cd' +################################ OpenID Auth ################################# +MITX_FEATURES['AUTH_USE_OPENID'] = True +MITX_FEATURES['BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'] = True + +INSTALLED_APPS += ('external_auth',) +INSTALLED_APPS += ('django_openid_auth',) + +OPENID_CREATE_USERS = False +OPENID_UPDATE_DETAILS_FROM_SREG = True +OPENID_SSO_SERVER_URL = 'https://www.google.com/accounts/o8/id' # TODO: accept more endpoints +OPENID_USE_AS_ADMIN_LOGIN = False + ################################ DEBUG TOOLBAR ################################# INSTALLED_APPS += ('debug_toolbar',) MIDDLEWARE_CLASSES += ('debug_toolbar.middleware.DebugToolbarMiddleware',) diff --git a/lms/envs/dev_ike.py b/lms/envs/dev_ike.py index 79ae3354ac..fb7d980550 100644 --- a/lms/envs/dev_ike.py +++ b/lms/envs/dev_ike.py @@ -7,142 +7,110 @@ sessions. Assumes structure: /mitx # The location of this repo /log # Where we're going to write log files """ - -import socket - -if 'eecs1' in socket.gethostname(): - MITX_ROOT_URL = '/mitx2' - from .common import * from .logsettings import get_logger_config -from .dev import * - -if 'eecs1' in socket.gethostname(): - # MITX_ROOT_URL = '/mitx2' - MITX_ROOT_URL = 'https://eecs1.mit.edu/mitx2' - -#----------------------------------------------------------------------------- -# edx4edx content server - -EMAIL_BACKEND = 'django.core.mail.backends.smtp.EmailBackend' -MITX_FEATURES['REROUTE_ACTIVATION_EMAIL'] = 'ichuang@mit.edu' -EDX4EDX_ROOT = ENV_ROOT / "data/edx4edx" - -#EMAIL_BACKEND = 'django_ses.SESBackend' - -#----------------------------------------------------------------------------- -# ichuang DEBUG = True -ENABLE_MULTICOURSE = True # set to False to disable multicourse display (see lib.util.views.mitxhome) -QUICKEDIT = False +TEMPLATE_DEBUG = True -MAKO_TEMPLATES['course'] = [DATA_DIR, EDX4EDX_ROOT ] +MITX_FEATURES['DISABLE_START_DATES'] = True -#MITX_FEATURES['USE_DJANGO_PIPELINE'] = False -MITX_FEATURES['DISPLAY_HISTOGRAMS_TO_STAFF'] = False -MITX_FEATURES['DISPLAY_EDIT_LINK'] = True -MITX_FEATURES['DEBUG_LEVEL'] = 10 # 0 = lowest level, least verbose, 255 = max level, most verbose +WIKI_ENABLED = True -COURSE_SETTINGS = {'6.002x_Fall_2012': {'number' : '6.002x', - 'title' : 'Circuits and Electronics', - 'xmlpath': '/6002x-fall-2012/', - 'active' : True, - 'default_chapter' : 'Week_1', - 'default_section' : 'Administrivia_and_Circuit_Elements', - 'location': 'i4x://edx/6002xs12/course/6.002x_Fall_2012', - }, - '8.02_Spring_2013': {'number' : '8.02x', - 'title' : 'Electricity & Magnetism', - 'xmlpath': '/802x/', - 'github_url': 'https://github.com/MITx/8.02x', - 'active' : True, - 'default_chapter' : 'Introduction', - 'default_section' : 'Introduction_%28Lewin_2002%29', - }, - '6.189_Spring_2013': {'number' : '6.189x', - 'title' : 'IAP Python Programming', - 'xmlpath': '/6.189x/', - 'github_url': 'https://github.com/MITx/6.189x', - 'active' : True, - 'default_chapter' : 'Week_1', - 'default_section' : 'Variables_and_Binding', - }, - '8.01_Fall_2012': {'number' : '8.01x', - 'title' : 'Mechanics', - 'xmlpath': '/8.01x/', - 'github_url': 'https://github.com/MITx/8.01x', - 'active': True, - 'default_chapter' : 'Mechanics_Online_Spring_2012', - 'default_section' : 'Introduction_to_the_course', - 'location': 'i4x://edx/6002xs12/course/8.01_Fall_2012', - }, - 'edx4edx': {'number' : 'edX.01', - 'title' : 'edx4edx: edX Author Course', - 'xmlpath': '/edx4edx/', - 'github_url': 'https://github.com/MITx/edx4edx', - 'active' : True, - 'default_chapter' : 'Introduction', - 'default_section' : 'edx4edx_Course', - 'location': 'i4x://edx/6002xs12/course/edx4edx', - }, - '7.03x_Fall_2012': {'number' : '7.03x', - 'title' : 'Genetics', - 'xmlpath': '/7.03x/', - 'github_url': 'https://github.com/MITx/7.03x', - 'active' : True, - 'default_chapter' : 'Week_2', - 'default_section' : 'ps1_question_1', - }, - '3.091x_Fall_2012': {'number' : '3.091x', - 'title' : 'Introduction to Solid State Chemistry', - 'xmlpath': '/3.091x/', - 'github_url': 'https://github.com/MITx/3.091x', - 'active' : True, - 'default_chapter' : 'Week_1', - 'default_section' : 'Problem_Set_1', - }, - '18.06x_Linear_Algebra': {'number' : '18.06x', - 'title' : 'Linear Algebra', - 'xmlpath': '/18.06x/', - 'github_url': 'https://github.com/MITx/18.06x', - 'default_chapter' : 'Unit_1', - 'default_section' : 'Midterm_1', - 'active' : True, - }, - '6.00x_Fall_2012': {'number' : '6.00x', - 'title' : 'Introduction to Computer Science and Programming', - 'xmlpath': '/6.00x/', - 'github_url': 'https://github.com/MITx/6.00x', - 'active' : True, - 'default_chapter' : 'Week_0', - 'default_section' : 'Problem_Set_0', - 'location': 'i4x://edx/6002xs12/course/6.00x_Fall_2012', - }, - '7.00x_Fall_2012': {'number' : '7.00x', - 'title' : 'Introduction to Biology', - 'xmlpath': '/7.00x/', - 'github_url': 'https://github.com/MITx/7.00x', - 'active' : True, - 'default_chapter' : 'Unit 1', - 'default_section' : 'Introduction', - }, - } +LOGGING = get_logger_config(ENV_ROOT / "log", + logging_env="dev", + tracking_filename="tracking.log", + debug=True) -#----------------------------------------------------------------------------- +DATABASES = { + 'default': { + 'ENGINE': 'django.db.backends.sqlite3', + 'NAME': ENV_ROOT / "db" / "mitx.db", + } +} -MIDDLEWARE_CLASSES = MIDDLEWARE_CLASSES + ( - 'ssl_auth.ssl_auth.NginxProxyHeaderMiddleware', # ssl authentication behind nginx proxy - ) +CACHES = { + # This is the cache used for most things. Askbot will not work without a + # functioning cache -- it relies on caching to load its settings in places. + # In staging/prod envs, the sessions also live here. + 'default': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'LOCATION': 'mitx_loc_mem_cache', + 'KEY_FUNCTION': 'util.memcache.safe_key', + }, -AUTHENTICATION_BACKENDS = ( - 'ssl_auth.ssl_auth.SSLLoginBackend', - 'django.contrib.auth.backends.ModelBackend', - ) + # The general cache is what you get if you use our util.cache. It's used for + # things like caching the course.xml file for different A/B test groups. + # We set it to be a DummyCache to force reloading of course.xml in dev. + # In staging environments, we would grab VERSION from data uploaded by the + # push process. + 'general': { + 'BACKEND': 'django.core.cache.backends.dummy.DummyCache', + 'KEY_PREFIX': 'general', + 'VERSION': 4, + 'KEY_FUNCTION': 'util.memcache.safe_key', + } +} -INSTALLED_APPS = INSTALLED_APPS + ( - 'ssl_auth', - ) +# Dummy secret key for dev +SECRET_KEY = '85920908f28904ed733fe576320db18cabd7b6cd' -LOGIN_REDIRECT_URL = MITX_ROOT_URL + '/' -LOGIN_URL = MITX_ROOT_URL + '/' +################################ OpenID Auth ################################# +MITX_FEATURES['AUTH_USE_OPENID'] = True + +INSTALLED_APPS += ('external_auth',) +INSTALLED_APPS += ('django_openid_auth',) +#INSTALLED_APPS += ('ssl_auth',) + +#MIDDLEWARE_CLASSES += ( +# #'ssl_auth.ssl_auth.NginxProxyHeaderMiddleware', # ssl authentication behind nginx proxy +# ) + +#AUTHENTICATION_BACKENDS = ( +# 'django_openid_auth.auth.OpenIDBackend', +# 'django.contrib.auth.backends.ModelBackend', +# ) + +OPENID_CREATE_USERS = False +OPENID_UPDATE_DETAILS_FROM_SREG = True +OPENID_SSO_SERVER_URL = 'https://www.google.com/accounts/o8/id' +OPENID_USE_AS_ADMIN_LOGIN = False +#import external_auth.views as edXauth +#OPENID_RENDER_FAILURE = edXauth.edXauth_openid + +################################ DEBUG TOOLBAR ################################# +INSTALLED_APPS += ('debug_toolbar',) +MIDDLEWARE_CLASSES += ('debug_toolbar.middleware.DebugToolbarMiddleware',) +INTERNAL_IPS = ('127.0.0.1',) + +DEBUG_TOOLBAR_PANELS = ( + 'debug_toolbar.panels.version.VersionDebugPanel', + 'debug_toolbar.panels.timer.TimerDebugPanel', + 'debug_toolbar.panels.settings_vars.SettingsVarsDebugPanel', + 'debug_toolbar.panels.headers.HeaderDebugPanel', + 'debug_toolbar.panels.request_vars.RequestVarsDebugPanel', + 'debug_toolbar.panels.sql.SQLDebugPanel', + 'debug_toolbar.panels.signals.SignalDebugPanel', + 'debug_toolbar.panels.logger.LoggingPanel', + +# Enabling the profiler has a weird bug as of django-debug-toolbar==0.9.4 and +# Django=1.3.1/1.4 where requests to views get duplicated (your method gets +# hit twice). So you can uncomment when you need to diagnose performance +# problems, but you shouldn't leave it on. +# 'debug_toolbar.panels.profiling.ProfilingDebugPanel', +) + +############################ FILE UPLOADS (ASKBOT) ############################# +DEFAULT_FILE_STORAGE = 'django.core.files.storage.FileSystemStorage' +MEDIA_ROOT = ENV_ROOT / "uploads" +MEDIA_URL = "/static/uploads/" +STATICFILES_DIRS.append(("uploads", MEDIA_ROOT)) +FILE_UPLOAD_TEMP_DIR = ENV_ROOT / "uploads" +FILE_UPLOAD_HANDLERS = ( + 'django.core.files.uploadhandler.MemoryFileUploadHandler', + 'django.core.files.uploadhandler.TemporaryFileUploadHandler', +) + +########################### PIPELINE ################################# + +PIPELINE_SASS_ARGUMENTS = '-r {proj_dir}/static/sass/bourbon/lib/bourbon.rb'.format(proj_dir=PROJECT_ROOT) diff --git a/lms/templates/extauth_failure.html b/lms/templates/extauth_failure.html new file mode 100644 index 0000000000..fa53ab1084 --- /dev/null +++ b/lms/templates/extauth_failure.html @@ -0,0 +1,11 @@ + + + + OpenID failed + + +

OpenID failed

+

${message}

+ + diff --git a/lms/templates/index.html b/lms/templates/index.html index d8b0394927..dcc18d4de1 100644 --- a/lms/templates/index.html +++ b/lms/templates/index.html @@ -144,3 +144,12 @@ + +% if show_signup_immediately is not UNDEFINED: + +% endif diff --git a/lms/templates/login_modal.html b/lms/templates/login_modal.html index 393e76ee78..e2a2754226 100644 --- a/lms/templates/login_modal.html +++ b/lms/templates/login_modal.html @@ -27,6 +27,9 @@ Not enrolled? Forgot password?

+

+ login via openid +

diff --git a/lms/templates/signup_modal.html b/lms/templates/signup_modal.html index aef90ab0f2..346027418d 100644 --- a/lms/templates/signup_modal.html +++ b/lms/templates/signup_modal.html @@ -19,6 +19,7 @@
+ % if has_extauth_info is UNDEFINED: @@ -27,6 +28,18 @@ + % else: +

Welcome ${extauth_email}


+ + +

Enter a public username:

+ + + + + + + % endif
@@ -93,11 +106,13 @@
+ % if has_extauth_info is UNDEFINED: + % endif
diff --git a/lms/urls.py b/lms/urls.py index 1c4a065e2b..8c36857ee3 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -160,12 +160,22 @@ if settings.DEBUG: ## Jasmine urlpatterns=urlpatterns + (url(r'^_jasmine/', include('django_jasmine.urls')),) +if settings.MITX_FEATURES.get('AUTH_USE_OPENID'): + urlpatterns += ( + url(r'^openid/login/$', 'django_openid_auth.views.login_begin', name='openid-login'), + url(r'^openid/complete/$', 'external_auth.views.edXauth_openid_login_complete', name='openid-complete'), + url(r'^openid/logo.gif$', 'django_openid_auth.views.logo', name='openid-logo'), + ) + urlpatterns += ( + url(r'^extauth/$', 'external_auth.views.edXauth_signup', name='extauth-signup'), + ) + # urlpatterns += (url(r'^openid/', include('django_openid_auth.urls')),) + urlpatterns = patterns(*urlpatterns) if settings.DEBUG: urlpatterns += static(settings.STATIC_URL, document_root=settings.STATIC_ROOT) - #Custom error pages handler404 = 'static_template_view.views.render_404' handler500 = 'static_template_view.views.render_500' diff --git a/requirements.txt b/requirements.txt index 46c822642e..33b2bfeb05 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,6 +8,7 @@ lxml boto mako python-memcached +python-openid path.py django_debug_toolbar -e git://github.com/MITx/django-pipeline.git#egg=django-pipeline @@ -37,6 +38,7 @@ django-jasmine django-keyedcache django-mako django-masquerade +django-openid-auth django-robots django-ses django-storages From 4874fa51efe4e09c16221c3319653c96778887d6 Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 1 Aug 2012 15:00:27 -0400 Subject: [PATCH 02/22] remove cruft from student.views --- common/djangoapps/student/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 692b135ff6..3ba83f42bb 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -23,7 +23,6 @@ from django.http import HttpResponse, Http404 from django.shortcuts import redirect from mitxmako.shortcuts import render_to_response, render_to_string from django.core.urlresolvers import reverse -#from BeautifulSoup import BeautifulSoup from bs4 import BeautifulSoup from django.core.cache import cache From 7b725a075ca9d73a13f1610aa48f6d7d1fd990e9 Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 1 Aug 2012 17:57:21 -0400 Subject: [PATCH 03/22] external_auth djangoapps files --- common/djangoapps/external_auth/__init__.py | 0 common/djangoapps/external_auth/admin.py | 8 ++ common/djangoapps/external_auth/models.py | 29 +++++ common/djangoapps/external_auth/views.py | 137 ++++++++++++++++++++ 4 files changed, 174 insertions(+) create mode 100644 common/djangoapps/external_auth/__init__.py create mode 100644 common/djangoapps/external_auth/admin.py create mode 100644 common/djangoapps/external_auth/models.py create mode 100644 common/djangoapps/external_auth/views.py diff --git a/common/djangoapps/external_auth/__init__.py b/common/djangoapps/external_auth/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/external_auth/admin.py b/common/djangoapps/external_auth/admin.py new file mode 100644 index 0000000000..343492bca7 --- /dev/null +++ b/common/djangoapps/external_auth/admin.py @@ -0,0 +1,8 @@ +''' +django admin pages for courseware model +''' + +from external_auth.models import * +from django.contrib import admin + +admin.site.register(ExternalAuthMap) diff --git a/common/djangoapps/external_auth/models.py b/common/djangoapps/external_auth/models.py new file mode 100644 index 0000000000..b8b167d25d --- /dev/null +++ b/common/djangoapps/external_auth/models.py @@ -0,0 +1,29 @@ +""" +WE'RE USING MIGRATIONS! + +If you make changes to this model, be sure to create an appropriate migration +file and check it in at the same time as your model changes. To do that, + +1. Go to the mitx dir +2. django-admin.py schemamigration student --auto --settings=lms.envs.dev --pythonpath=. description_of_your_change +3. Add the migration file created in mitx/common/djangoapps/external_auth/migrations/ +""" + +from django.db import models +from django.contrib.auth.models import User + +class ExternalAuthMap(models.Model): + external_id = models.CharField(max_length=255, db_index=True, unique=True) + external_domain = models.CharField(max_length=255, db_index=True) + external_credentials = models.TextField(blank=True) # JSON dictionary + external_email = models.CharField(max_length=255, db_index=True) + external_name = models.CharField(blank=True,max_length=255, db_index=True) + user = models.OneToOneField(User, unique=True, db_index=True, null=True) + internal_password = models.CharField(blank=True, max_length=31) # randomly generated + dtcreated = models.DateTimeField('creation date',auto_now_add=True) + dtsignup = models.DateTimeField('signup date',null=True) # set after signup + + def __unicode__(self): + s = "[%s] = (%s / %s)" % (self.external_id, self.external_name, self.external_email) + return s + diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py new file mode 100644 index 0000000000..57131f21da --- /dev/null +++ b/common/djangoapps/external_auth/views.py @@ -0,0 +1,137 @@ +# from pprint import pprint + +import json +import logging +import random +import string + +from external_auth.models import ExternalAuthMap + +from django.conf import settings +from django.contrib.auth import REDIRECT_FIELD_NAME, authenticate, login +from django.contrib.auth.models import Group +from django.contrib.auth.models import User + +from django.core.urlresolvers import reverse +from django.http import HttpResponse, HttpResponseRedirect +from django.shortcuts import render_to_response +from django.shortcuts import redirect +from django.template import RequestContext +from mitxmako.shortcuts import render_to_response, render_to_string +try: + from django.views.decorators.csrf import csrf_exempt +except ImportError: + from django.contrib.csrf.middleware import csrf_exempt +from django_future.csrf import ensure_csrf_cookie +from util.cache import cache_if_anonymous + +#from django_openid_auth import views as openid_views +from django_openid_auth import auth as openid_auth +from openid.consumer.consumer import (Consumer, SUCCESS, CANCEL, FAILURE) +import django_openid_auth + +import student.views as student_views + +log = logging.getLogger("mitx.external_auth") + +@csrf_exempt +def default_render_failure(request, message, status=403, template_name='extauth_failure.html', exception=None): + """Render an Openid error page to the user.""" + + message = "In openid_failure " + message + print "in openid_failure: " + data = render_to_string( template_name, dict(message=message, exception=exception)) + return HttpResponse(data, status=status) + +@csrf_exempt +def edXauth_openid_login_complete(request, redirect_field_name=REDIRECT_FIELD_NAME, render_failure=None): + """Complete the openid login process""" + + redirect_to = request.REQUEST.get(redirect_field_name, '') + render_failure = render_failure or \ + getattr(settings, 'OPENID_RENDER_FAILURE', None) or \ + default_render_failure + + openid_response = django_openid_auth.views.parse_openid_response(request) + if not openid_response: + return render_failure(request, 'This is an OpenID relying party endpoint.') + + if openid_response.status == SUCCESS: + external_id = openid_response.identity_url + oid_backend = openid_auth.OpenIDBackend() + details = oid_backened = oid_backend._extract_user_details(openid_response) + + log.debug('openid success, details=%s' % details) + + # see if we have a map from this external_id to an edX username + try: + eamap = ExternalAuthMap.objects.get(external_id=external_id) + log.debug('Found eamap=%s' % eamap) + except ExternalAuthMap.DoesNotExist: + # go render form for creating edX user + eamap = ExternalAuthMap(external_id = external_id, + external_domain = "openid:%s" % settings.OPENID_SSO_SERVER_URL, + external_credentials = json.dumps(details), + ) + eamap.external_email = details.get('email','') + eamap.external_name = '%s %s' % (details.get('first_name',''),details.get('last_name','')) + + def GenPasswd(length=12, chars=string.letters + string.digits): + return ''.join([random.choice(chars) for i in range(length)]) + eamap.internal_password = GenPasswd() + log.debug('created eamap=%s' % eamap) + + eamap.save() + + internal_user = eamap.user + if internal_user is None: + log.debug('ExtAuth: no user for %s yet, doing signup' % eamap.external_email) + return edXauth_signup(request, eamap) + + uname = internal_user.username + user = authenticate(username=uname, password=eamap.internal_password) + if user is None: + log.warning("External Auth Login failed for %s / %s" % (uname,eamap.internal_password)) + return edXauth_signup(request, eamap) + + if not user.is_active: + log.warning("External Auth: user %s is not active" % (uname)) + # TODO: improve error page + return render_failure(request, 'Account not yet activated: please look for link in your email') + + login(request, user) + request.session.set_expiry(0) + student_views.try_change_enrollment(request) + log.info("Login success - {0} ({1})".format(user.username, user.email)) + return redirect('/') + + return render_failure(request, 'Openid failure') + +@ensure_csrf_cookie +@cache_if_anonymous +def edXauth_signup(request, eamap=None): + """ + Present form to complete for signup via external authentication. + Even though the user has external credentials, he/she still needs + to create an account on the edX system, and fill in the user + registration form. + + eamap is an ExteralAuthMap object, specifying the external user + for which to complete the signup. + """ + + if eamap is None: + pass + + request.session['ExternalAuthMap'] = eamap # save this for use by student.views.create_account + + context = {'has_extauth_info': True, + 'show_signup_immediately' : True, + 'extauth_email': eamap.external_email, + 'extauth_username' : eamap.external_name.split(' ')[0], + 'extauth_name': eamap.external_name, + } + + log.debug('ExtAuth: doing signup for %s' % eamap.external_email) + + return student_views.main_index(extra_context=context) From ec78e1a126fe9a2603a7a8b75980d1b0c6b21aa5 Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 1 Aug 2012 20:19:33 -0400 Subject: [PATCH 04/22] add missing trailing slash --- lms/templates/login_modal.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/login_modal.html b/lms/templates/login_modal.html index e2a2754226..c89931695c 100644 --- a/lms/templates/login_modal.html +++ b/lms/templates/login_modal.html @@ -28,7 +28,7 @@ Forgot password?

- login via openid + login via openid

From 102d4906d0d1c94aec6d2854e9e46c6707c0d09e Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 1 Aug 2012 22:41:27 -0400 Subject: [PATCH 05/22] fix problem with add_histogram (expects module as second argument) --- lms/djangoapps/courseware/module_render.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 80a4ef90fc..16921d1d50 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -184,7 +184,7 @@ def get_module(user, request, location, student_module_cache, position=None): ) if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF') and user.is_staff: - module.get_html = add_histogram(module.get_html) + module.get_html = add_histogram(module.get_html, module) # If StudentModule for this instance wasn't already in the database, # and this isn't a guest user, create it. From a759850e3e45a04e5953320fdb001e98df6a81be Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 1 Aug 2012 22:42:06 -0400 Subject: [PATCH 06/22] add SSL / MIT certificates auth; clean up external_auth.views --- common/djangoapps/external_auth/views.py | 162 +++++++++---- common/djangoapps/student/views.py | 4 + lms/djangoapps/ssl_auth/__init__.py | 0 lms/djangoapps/ssl_auth/ssl_auth.py | 290 ----------------------- lms/envs/dev.py | 3 + 5 files changed, 124 insertions(+), 335 deletions(-) delete mode 100644 lms/djangoapps/ssl_auth/__init__.py delete mode 100755 lms/djangoapps/ssl_auth/ssl_auth.py diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 57131f21da..5004d614d5 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -1,8 +1,7 @@ -# from pprint import pprint - import json import logging import random +import re import string from external_auth.models import ExternalAuthMap @@ -25,7 +24,6 @@ except ImportError: from django_future.csrf import ensure_csrf_cookie from util.cache import cache_if_anonymous -#from django_openid_auth import views as openid_views from django_openid_auth import auth as openid_auth from openid.consumer.consumer import (Consumer, SUCCESS, CANCEL, FAILURE) import django_openid_auth @@ -43,6 +41,12 @@ def default_render_failure(request, message, status=403, template_name='extauth_ data = render_to_string( template_name, dict(message=message, exception=exception)) return HttpResponse(data, status=status) +#----------------------------------------------------------------------------- +# Openid + +def GenPasswd(length=12, chars=string.letters + string.digits): + return ''.join([random.choice(chars) for i in range(length)]) + @csrf_exempt def edXauth_openid_login_complete(request, redirect_field_name=REDIRECT_FIELD_NAME, render_failure=None): """Complete the openid login process""" @@ -63,50 +67,62 @@ def edXauth_openid_login_complete(request, redirect_field_name=REDIRECT_FIELD_N log.debug('openid success, details=%s' % details) - # see if we have a map from this external_id to an edX username - try: - eamap = ExternalAuthMap.objects.get(external_id=external_id) - log.debug('Found eamap=%s' % eamap) - except ExternalAuthMap.DoesNotExist: - # go render form for creating edX user - eamap = ExternalAuthMap(external_id = external_id, - external_domain = "openid:%s" % settings.OPENID_SSO_SERVER_URL, - external_credentials = json.dumps(details), - ) - eamap.external_email = details.get('email','') - eamap.external_name = '%s %s' % (details.get('first_name',''),details.get('last_name','')) - - def GenPasswd(length=12, chars=string.letters + string.digits): - return ''.join([random.choice(chars) for i in range(length)]) - eamap.internal_password = GenPasswd() - log.debug('created eamap=%s' % eamap) - - eamap.save() - - internal_user = eamap.user - if internal_user is None: - log.debug('ExtAuth: no user for %s yet, doing signup' % eamap.external_email) - return edXauth_signup(request, eamap) - - uname = internal_user.username - user = authenticate(username=uname, password=eamap.internal_password) - if user is None: - log.warning("External Auth Login failed for %s / %s" % (uname,eamap.internal_password)) - return edXauth_signup(request, eamap) - - if not user.is_active: - log.warning("External Auth: user %s is not active" % (uname)) - # TODO: improve error page - return render_failure(request, 'Account not yet activated: please look for link in your email') - - login(request, user) - request.session.set_expiry(0) - student_views.try_change_enrollment(request) - log.info("Login success - {0} ({1})".format(user.username, user.email)) - return redirect('/') - + return edXauth_external_login_or_signup(request, + external_id, + "openid:%s" % settings.OPENID_SSO_SERVER_URL, + details, + details.get('email',''), + '%s %s' % (details.get('first_name',''),details.get('last_name','')) + ) + return render_failure(request, 'Openid failure') + +#----------------------------------------------------------------------------- +# generic external auth login or signup + +def edXauth_external_login_or_signup(request, external_id, external_domain, credentials, email, fullname): + # see if we have a map from this external_id to an edX username + try: + eamap = ExternalAuthMap.objects.get(external_id=external_id) + log.debug('Found eamap=%s' % eamap) + except ExternalAuthMap.DoesNotExist: + # go render form for creating edX user + eamap = ExternalAuthMap(external_id = external_id, + external_domain = external_domain, + external_credentials = json.dumps(credentials), + ) + eamap.external_email = email + eamap.external_name = fullname + eamap.internal_password = GenPasswd() + log.debug('created eamap=%s' % eamap) + + eamap.save() + + internal_user = eamap.user + if internal_user is None: + log.debug('ExtAuth: no user for %s yet, doing signup' % eamap.external_email) + return edXauth_signup(request, eamap) + uname = internal_user.username + user = authenticate(username=uname, password=eamap.internal_password) + if user is None: + log.warning("External Auth Login failed for %s / %s" % (uname,eamap.internal_password)) + return edXauth_signup(request, eamap) + + if not user.is_active: + log.warning("External Auth: user %s is not active" % (uname)) + # TODO: improve error page + return render_failure(request, 'Account not yet activated: please look for link in your email') + + login(request, user) + request.session.set_expiry(0) + student_views.try_change_enrollment(request) + log.info("Login success - {0} ({1})".format(user.username, user.email)) + return redirect('/') + +#----------------------------------------------------------------------------- +# generic external auth signup + @ensure_csrf_cookie @cache_if_anonymous def edXauth_signup(request, eamap=None): @@ -135,3 +151,59 @@ def edXauth_signup(request, eamap=None): log.debug('ExtAuth: doing signup for %s' % eamap.external_email) return student_views.main_index(extra_context=context) + +#----------------------------------------------------------------------------- +# MIT SSL + +def ssl_dn_extract_info(dn): + ''' + Extract username, email address (may be anyuser@anydomain.com) and full name + from the SSL DN string. Return (user,email,fullname) if successful, and None + otherwise. + ''' + ss = re.search('/emailAddress=(.*)@([^/]+)', dn) + if ss: + user = ss.group(1) + email = "%s@%s" % (user, ss.group(2)) + else: + return None + ss = re.search('/CN=([^/]+)/', dn) + if ss: + fullname = ss.group(1) + else: + return None + return (user, email, fullname) + +@csrf_exempt +def edXauth_ssl_login(request): + """ + This is called by student.views.index when MITX_FEATURES['AUTH_USE_MIT_CERTIFICATES'] = True + + Used for MIT user authentication. This presumes the web server (nginx) has been configured + to require specific client certificates. + + If the incoming protocol is HTTPS (SSL) then authenticate via client certificate. + The certificate provides user email and fullname; this populates the ExternalAuthMap. + The user is nevertheless still asked to complete the edX signup. + + Else continues on with student.views.main_index, and no authentication. + """ + certkey = "SSL_CLIENT_S_DN" # specify the request.META field to use + + cert = request.META.get(certkey,'') + if not cert: + cert = request.META.get('HTTP_'+certkey,'') + if not cert: + cert = request._req.subprocess_env.get(certkey,'') # try the direct apache2 SSL key + if not cert: + # no certificate information - go onward to main index + return student_views.main_index() + + (user, email, fullname) = ssl_dn_extract_info(cert) + + return edXauth_external_login_or_signup(request, + external_id=email, + external_domain="ssl:MIT", + credentials=cert, + email=email, + fullname=fullname) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 3ba83f42bb..7937d67980 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -60,6 +60,10 @@ def index(request): if settings.COURSEWARE_ENABLED and request.user.is_authenticated(): return redirect(reverse('dashboard')) + if settings.MITX_FEATURES.get('AUTH_USE_MIT_CERTIFICATES'): + from external_auth.views import edXauth_ssl_login + return edXauth_ssl_login(request) + return main_index() def main_index(extra_context = {}): diff --git a/lms/djangoapps/ssl_auth/__init__.py b/lms/djangoapps/ssl_auth/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/lms/djangoapps/ssl_auth/ssl_auth.py b/lms/djangoapps/ssl_auth/ssl_auth.py deleted file mode 100755 index adbb2bf94d..0000000000 --- a/lms/djangoapps/ssl_auth/ssl_auth.py +++ /dev/null @@ -1,290 +0,0 @@ -""" -User authentication backend for ssl (no pw required) -""" - -from django.conf import settings -from django.contrib import auth -from django.contrib.auth.models import User, check_password -from django.contrib.auth.backends import ModelBackend -from django.contrib.auth.middleware import RemoteUserMiddleware -from django.core.exceptions import ImproperlyConfigured -import os -import string -import re -from random import choice - -from student.models import UserProfile - -#----------------------------------------------------------------------------- - - -def ssl_dn_extract_info(dn): - ''' - Extract username, email address (may be anyuser@anydomain.com) and full name - from the SSL DN string. Return (user,email,fullname) if successful, and None - otherwise. - ''' - ss = re.search('/emailAddress=(.*)@([^/]+)', dn) - if ss: - user = ss.group(1) - email = "%s@%s" % (user, ss.group(2)) - else: - return None - ss = re.search('/CN=([^/]+)/', dn) - if ss: - fullname = ss.group(1) - else: - return None - return (user, email, fullname) - - -def check_nginx_proxy(request): - ''' - Check for keys in the HTTP header (META) to se if we are behind an ngix reverse proxy. - If so, get user info from the SSL DN string and return that, as (user,email,fullname) - ''' - m = request.META - if m.has_key('HTTP_X_REAL_IP'): # we're behind a nginx reverse proxy, which has already done ssl auth - if not m.has_key('HTTP_SSL_CLIENT_S_DN'): - return None - dn = m['HTTP_SSL_CLIENT_S_DN'] - return ssl_dn_extract_info(dn) - return None - -#----------------------------------------------------------------------------- - - -def get_ssl_username(request): - x = check_nginx_proxy(request) - if x: - return x[0] - env = request._req.subprocess_env - if env.has_key('SSL_CLIENT_S_DN_Email'): - email = env['SSL_CLIENT_S_DN_Email'] - user = email[:email.index('@')] - return user - return None - -#----------------------------------------------------------------------------- - - -class NginxProxyHeaderMiddleware(RemoteUserMiddleware): - ''' - Django "middleware" function for extracting user information from HTTP request. - - ''' - # this field is generated by nginx's reverse proxy - header = 'HTTP_SSL_CLIENT_S_DN' # specify the request.META field to use - - def process_request(self, request): - # AuthenticationMiddleware is required so that request.user exists. - if not hasattr(request, 'user'): - raise ImproperlyConfigured( - "The Django remote user auth middleware requires the" - " authentication middleware to be installed. Edit your" - " MIDDLEWARE_CLASSES setting to insert" - " 'django.contrib.auth.middleware.AuthenticationMiddleware'" - " before the RemoteUserMiddleware class.") - - #raise ImproperlyConfigured('[ProxyHeaderMiddleware] request.META=%s' % repr(request.META)) - - try: - username = request.META[self.header] # try the nginx META key first - except KeyError: - try: - env = request._req.subprocess_env # else try the direct apache2 SSL key - if env.has_key('SSL_CLIENT_S_DN'): - username = env['SSL_CLIENT_S_DN'] - else: - raise ImproperlyConfigured('no ssl key, env=%s' % repr(env)) - username = '' - except: - # If specified header doesn't exist then return (leaving - # request.user set to AnonymousUser by the - # AuthenticationMiddleware). - return - # If the user is already authenticated and that user is the user we are - # getting passed in the headers, then the correct user is already - # persisted in the session and we don't need to continue. - - #raise ImproperlyConfigured('[ProxyHeaderMiddleware] username=%s' % username) - - if request.user.is_authenticated(): - if request.user.username == self.clean_username(username, request): - #raise ImproperlyConfigured('%s already authenticated (%s)' % (username,request.user.username)) - return - # We are seeing this user for the first time in this session, attempt - # to authenticate the user. - #raise ImproperlyConfigured('calling auth.authenticate, remote_user=%s' % username) - user = auth.authenticate(remote_user=username) - if user: - # User is valid. Set request.user and persist user in the session - # by logging the user in. - request.user = user - if settings.DEBUG: print "[ssl_auth.ssl_auth.NginxProxyHeaderMiddleware] logging in user=%s" % user - auth.login(request, user) - - def clean_username(self, username, request): - ''' - username is the SSL DN string - extract the actual username from it and return - ''' - info = ssl_dn_extract_info(username) - if not info: - return None - (username, email, fullname) = info - return username - -#----------------------------------------------------------------------------- - - -class SSLLoginBackend(ModelBackend): - ''' - Django authentication back-end which auto-logs-in a user based on having - already authenticated with an MIT certificate (SSL). - ''' - def authenticate(self, username=None, password=None, remote_user=None): - - # remote_user is from the SSL_DN string. It will be non-empty only when - # the user has already passed the server authentication, which means - # matching with the certificate authority. - if not remote_user: - # no remote_user, so check username (but don't auto-create user) - if not username: - return None - return None # pass on to another authenticator backend - #raise ImproperlyConfigured("in SSLLoginBackend, username=%s, remote_user=%s" % (username,remote_user)) - try: - user = User.objects.get(username=username) # if user already exists don't create it - return user - except User.DoesNotExist: - return None - return None - - #raise ImproperlyConfigured("in SSLLoginBackend, username=%s, remote_user=%s" % (username,remote_user)) - #if not os.environ.has_key('HTTPS'): - # return None - #if not os.environ.get('HTTPS')=='on': # only use this back-end if HTTPS on - # return None - - def GenPasswd(length=8, chars=string.letters + string.digits): - return ''.join([choice(chars) for i in range(length)]) - - # convert remote_user to user, email, fullname - info = ssl_dn_extract_info(remote_user) - #raise ImproperlyConfigured("[SSLLoginBackend] looking up %s" % repr(info)) - if not info: - #raise ImproperlyConfigured("[SSLLoginBackend] remote_user=%s, info=%s" % (remote_user,info)) - return None - (username, email, fullname) = info - - try: - user = User.objects.get(username=username) # if user already exists don't create it - except User.DoesNotExist: - if not settings.DEBUG: - raise "User does not exist. Not creating user; potential schema consistency issues" - #raise ImproperlyConfigured("[SSLLoginBackend] creating %s" % repr(info)) - user = User(username=username, password=GenPasswd()) # create new User - user.is_staff = False - user.is_superuser = False - # get first, last name from fullname - name = fullname - if not name.count(' '): - user.first_name = " " - user.last_name = name - mn = '' - else: - user.first_name = name[:name.find(' ')] - ml = name[name.find(' '):].strip() - if ml.count(' '): - user.last_name = ml[ml.rfind(' '):] - mn = ml[:ml.rfind(' ')] - else: - user.last_name = ml - mn = '' - # set email - user.email = email - # cleanup last name - user.last_name = user.last_name.strip() - # save - user.save() - - # auto-create user profile - up = UserProfile(user=user) - up.name = fullname - up.save() - - #tui = user.get_profile() - #tui.middle_name = mn - #tui.role = 'Misc' - #tui.section = None # no section assigned at first - #tui.save() - # return None - return user - - def get_user(self, user_id): - #if not os.environ.has_key('HTTPS'): - # return None - #if not os.environ.get('HTTPS')=='on': # only use this back-end if HTTPS on - # return None - try: - return User.objects.get(pk=user_id) - except User.DoesNotExist: - return None - -#----------------------------------------------------------------------------- -# OLD! - - -class AutoLoginBackend: - def authenticate(self, username=None, password=None): - raise ImproperlyConfigured("in AutoLoginBackend, username=%s" % username) - if not os.environ.has_key('HTTPS'): - return None - if not os.environ.get('HTTPS') == 'on':# only use this back-end if HTTPS on - return None - - def GenPasswd(length=8, chars=string.letters + string.digits): - return ''.join([choice(chars) for i in range(length)]) - - try: - user = User.objects.get(username=username) - except User.DoesNotExist: - user = User(username=username, password=GenPasswd()) - user.is_staff = False - user.is_superuser = False - # get first, last name - name = os.environ.get('SSL_CLIENT_S_DN_CN').strip() - if not name.count(' '): - user.first_name = " " - user.last_name = name - mn = '' - else: - user.first_name = name[:name.find(' ')] - ml = name[name.find(' '):].strip() - if ml.count(' '): - user.last_name = ml[ml.rfind(' '):] - mn = ml[:ml.rfind(' ')] - else: - user.last_name = ml - mn = '' - # get email - user.email = os.environ.get('SSL_CLIENT_S_DN_Email') - # save - user.save() - tui = user.get_profile() - tui.middle_name = mn - tui.role = 'Misc' - tui.section = None# no section assigned at first - tui.save() - # return None - return user - - def get_user(self, user_id): - if not os.environ.has_key('HTTPS'): - return None - if not os.environ.get('HTTPS') == 'on':# only use this back-end if HTTPS on - return None - try: - return User.objects.get(pk=user_id) - except User.DoesNotExist: - return None diff --git a/lms/envs/dev.py b/lms/envs/dev.py index f9b7ba10a0..83bc596f1e 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -67,6 +67,9 @@ OPENID_UPDATE_DETAILS_FROM_SREG = True OPENID_SSO_SERVER_URL = 'https://www.google.com/accounts/o8/id' # TODO: accept more endpoints OPENID_USE_AS_ADMIN_LOGIN = False +################################ MIT Certificates SSL Auth ################################# +MITX_FEATURES['AUTH_USE_MIT_CERTIFICATES'] = True + ################################ DEBUG TOOLBAR ################################# INSTALLED_APPS += ('debug_toolbar',) MIDDLEWARE_CLASSES += ('debug_toolbar.middleware.DebugToolbarMiddleware',) From 4a0d0a08db20fe64fa0dceb08d157cce1cfaa026 Mon Sep 17 00:00:00 2001 From: ichuang Date: Wed, 1 Aug 2012 23:37:35 -0400 Subject: [PATCH 07/22] minor change so that SSL code doesn't interfere with non-nginx instances --- common/djangoapps/external_auth/views.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 5004d614d5..41062735d4 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -194,7 +194,10 @@ def edXauth_ssl_login(request): if not cert: cert = request.META.get('HTTP_'+certkey,'') if not cert: - cert = request._req.subprocess_env.get(certkey,'') # try the direct apache2 SSL key + try: + cert = request._req.subprocess_env.get(certkey,'') # try the direct apache2 SSL key + except Exception as err: + pass if not cert: # no certificate information - go onward to main index return student_views.main_index() From 727e51411f7e096c92745c3deb630bd77b2f119c Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 08:59:02 -0400 Subject: [PATCH 08/22] small change so that ssl authenticated user can logout to see main screen --- common/djangoapps/external_auth/views.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 41062735d4..55ff4b4194 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -80,7 +80,8 @@ def edXauth_openid_login_complete(request, redirect_field_name=REDIRECT_FIELD_N #----------------------------------------------------------------------------- # generic external auth login or signup -def edXauth_external_login_or_signup(request, external_id, external_domain, credentials, email, fullname): +def edXauth_external_login_or_signup(request, external_id, external_domain, credentials, email, fullname, + retfun=None): # see if we have a map from this external_id to an edX username try: eamap = ExternalAuthMap.objects.get(external_id=external_id) @@ -118,7 +119,10 @@ def edXauth_external_login_or_signup(request, external_id, external_domain, cred request.session.set_expiry(0) student_views.try_change_enrollment(request) log.info("Login success - {0} ({1})".format(user.username, user.email)) - return redirect('/') + if retfun is None: + return redirect('/') + return retfun() + #----------------------------------------------------------------------------- # generic external auth signup @@ -209,4 +213,5 @@ def edXauth_ssl_login(request): external_domain="ssl:MIT", credentials=cert, email=email, - fullname=fullname) + fullname=fullname, + retfun = student_views.main_index) From 23c3c5a6529db587e505aed01f908ea6684e3b8c Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 09:37:24 -0400 Subject: [PATCH 09/22] print -> log.debug, rename function from camel case --- common/djangoapps/external_auth/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 55ff4b4194..fdffaf0c1d 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -35,16 +35,16 @@ log = logging.getLogger("mitx.external_auth") @csrf_exempt def default_render_failure(request, message, status=403, template_name='extauth_failure.html', exception=None): """Render an Openid error page to the user.""" - message = "In openid_failure " + message - print "in openid_failure: " + log.debug(message) data = render_to_string( template_name, dict(message=message, exception=exception)) return HttpResponse(data, status=status) #----------------------------------------------------------------------------- # Openid -def GenPasswd(length=12, chars=string.letters + string.digits): +def edXauth_generate_password(length=12, chars=string.letters + string.digits): + """Generate internal password for externally authenticated user""" return ''.join([random.choice(chars) for i in range(length)]) @csrf_exempt @@ -94,7 +94,7 @@ def edXauth_external_login_or_signup(request, external_id, external_domain, cred ) eamap.external_email = email eamap.external_name = fullname - eamap.internal_password = GenPasswd() + eamap.internal_password = edXauth_generate_password() log.debug('created eamap=%s' % eamap) eamap.save() From b2e9d980ff4840539fd593f43c175f1040221d3b Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 09:42:26 -0400 Subject: [PATCH 10/22] don't overwrite oid_backend --- common/djangoapps/external_auth/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index fdffaf0c1d..827f2bc4c5 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -63,7 +63,7 @@ def edXauth_openid_login_complete(request, redirect_field_name=REDIRECT_FIELD_N if openid_response.status == SUCCESS: external_id = openid_response.identity_url oid_backend = openid_auth.OpenIDBackend() - details = oid_backened = oid_backend._extract_user_details(openid_response) + details = oid_backend._extract_user_details(openid_response) log.debug('openid success, details=%s' % details) From f2a9110bdaa804bf20a91d9020113f2657d05c76 Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 09:56:33 -0400 Subject: [PATCH 11/22] change model to have external_id and external_domain be unique_together --- common/djangoapps/external_auth/models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/external_auth/models.py b/common/djangoapps/external_auth/models.py index b8b167d25d..e43b306bbb 100644 --- a/common/djangoapps/external_auth/models.py +++ b/common/djangoapps/external_auth/models.py @@ -13,7 +13,9 @@ from django.db import models from django.contrib.auth.models import User class ExternalAuthMap(models.Model): - external_id = models.CharField(max_length=255, db_index=True, unique=True) + class Meta: + unique_together = (('external_id', 'external_domain'), ) + external_id = models.CharField(max_length=255, db_index=True) external_domain = models.CharField(max_length=255, db_index=True) external_credentials = models.TextField(blank=True) # JSON dictionary external_email = models.CharField(max_length=255, db_index=True) From 613c53a7109b9162acc439e202b3430f099ee35f Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 10:05:26 -0400 Subject: [PATCH 12/22] slight cleanup, no need to import all of django_openid_auth --- common/djangoapps/external_auth/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 827f2bc4c5..740b4ed1ac 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -26,7 +26,7 @@ from util.cache import cache_if_anonymous from django_openid_auth import auth as openid_auth from openid.consumer.consumer import (Consumer, SUCCESS, CANCEL, FAILURE) -import django_openid_auth +import django_openid_auth.views as openid_views import student.views as student_views @@ -56,7 +56,7 @@ def edXauth_openid_login_complete(request, redirect_field_name=REDIRECT_FIELD_N getattr(settings, 'OPENID_RENDER_FAILURE', None) or \ default_render_failure - openid_response = django_openid_auth.views.parse_openid_response(request) + openid_response = openid_views.parse_openid_response(request) if not openid_response: return render_failure(request, 'This is an OpenID relying party endpoint.') From 3eff9ffecd2382ba0bad9b877376388daffa7a8e Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 13:28:52 -0400 Subject: [PATCH 13/22] match external_domain as well when retrieving ExternalAuthMap objects --- common/djangoapps/external_auth/views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 740b4ed1ac..d00a0a7182 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -84,7 +84,9 @@ def edXauth_external_login_or_signup(request, external_id, external_domain, cred retfun=None): # see if we have a map from this external_id to an edX username try: - eamap = ExternalAuthMap.objects.get(external_id=external_id) + eamap = ExternalAuthMap.objects.get(external_id = external_id, + external_domain = external_domain, + ) log.debug('Found eamap=%s' % eamap) except ExternalAuthMap.DoesNotExist: # go render form for creating edX user From a7103ff8932ddfdcd10e844aa71fd3901690de47 Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 13:39:12 -0400 Subject: [PATCH 14/22] switch to PascalCase, remove unnecessary assignment --- common/djangoapps/student/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 7937d67980..35ce225011 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -280,14 +280,14 @@ def create_account(request, post_override=None): # if doing signup for an external authorization, then get email, password, name from the eamap # don't use the ones from the form, since the user could have hacked those - doExternalAuth = 'ExternalAuthMap' in request.session - if doExternalAuth: + DoExternalAuth = 'ExternalAuthMap' in request.session + if DoExternalAuth: eamap = request.session['ExternalAuthMap'] email = eamap.external_email name = eamap.external_name password = eamap.internal_password post_vars = dict(post_vars.items()) - post_vars.update(dict(email=email, name=name, password=password, username=post_vars['username'])) + post_vars.update(dict(email=email, name=name, password=password)) log.debug('extauth test: post_vars = %s' % post_vars) # Confirm we have a properly formed request @@ -411,7 +411,7 @@ def create_account(request, post_override=None): try_change_enrollment(request) - if doExternalAuth: + if DoExternalAuth: eamap.user = login_user eamap.dtsignup = datetime.datetime.now() eamap.save() From 46bc7bc499fb63005d5225a465470c9d7481aa3d Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 13:44:37 -0400 Subject: [PATCH 15/22] remove unnecessary hidden fields --- lms/templates/signup_modal.html | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lms/templates/signup_modal.html b/lms/templates/signup_modal.html index 346027418d..1510eb407b 100644 --- a/lms/templates/signup_modal.html +++ b/lms/templates/signup_modal.html @@ -30,15 +30,9 @@ % else:

Welcome ${extauth_email}


- -

Enter a public username:

- - - - % endif From 10b2c212b6d9159acf430f8d65738471292eea5c Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 13:52:25 -0400 Subject: [PATCH 16/22] fix javascript for signup modal .click() in index.html for safari --- lms/templates/index.html | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/lms/templates/index.html b/lms/templates/index.html index dcc18d4de1..4255251604 100644 --- a/lms/templates/index.html +++ b/lms/templates/index.html @@ -148,7 +148,26 @@ % if show_signup_immediately is not UNDEFINED: From dff380e8076a49a82d10bda267d89e9d7259133d Mon Sep 17 00:00:00 2001 From: ichuang Date: Thu, 2 Aug 2012 14:19:56 -0400 Subject: [PATCH 17/22] cleanup lms urls (remove cruft from debugging openid) --- lms/urls.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lms/urls.py b/lms/urls.py index 8c36857ee3..35a71920a5 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -166,10 +166,6 @@ if settings.MITX_FEATURES.get('AUTH_USE_OPENID'): url(r'^openid/complete/$', 'external_auth.views.edXauth_openid_login_complete', name='openid-complete'), url(r'^openid/logo.gif$', 'django_openid_auth.views.logo', name='openid-logo'), ) - urlpatterns += ( - url(r'^extauth/$', 'external_auth.views.edXauth_signup', name='extauth-signup'), - ) - # urlpatterns += (url(r'^openid/', include('django_openid_auth.urls')),) urlpatterns = patterns(*urlpatterns) From 987b9c11a94372f6d4c888755a1ce5ff5a036e30 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 2 Aug 2012 14:05:42 -0400 Subject: [PATCH 18/22] Use url_name for chapters and sections in lms views * got rid of the hackish conversions between ' ' and '_' * use url_name and display_name where appropriate * update templates to match. --- common/lib/xmodule/xmodule/x_module.py | 2 ++ lms/djangoapps/courseware/grades.py | 33 ++++++++++++++-------- lms/djangoapps/courseware/module_render.py | 28 ++++++++++-------- lms/djangoapps/courseware/views.py | 29 ++++--------------- lms/templates/accordion.html | 6 ++-- lms/templates/profile.html | 28 +++++++++--------- 6 files changed, 62 insertions(+), 64 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index ac6b5db5a4..1d16849d67 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -339,6 +339,8 @@ class XModuleDescriptor(Plugin, HTMLSnippet): module display_name: The name to use for displaying this module to the user + url_name: The name to use for this module in urls and other places + where a unique name is needed. format: The format of this module ('Homework', 'Lab', etc) graded (bool): Whether this module is should be graded or not start (string): The date for which this module will be available diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 5a817e3d6c..717cbde140 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -12,18 +12,23 @@ _log = logging.getLogger("mitx.courseware") def grade_sheet(student, course, grader, student_module_cache): """ - This pulls a summary of all problems in the course. It returns a dictionary with two datastructures: + This pulls a summary of all problems in the course. It returns a dictionary + with two datastructures: - - courseware_summary is a summary of all sections with problems in the course. It is organized as an array of chapters, - each containing an array of sections, each containing an array of scores. This contains information for graded and ungraded - problems, and is good for displaying a course summary with due dates, etc. + - courseware_summary is a summary of all sections with problems in the + course. It is organized as an array of chapters, each containing an array of + sections, each containing an array of scores. This contains information for + graded and ungraded problems, and is good for displaying a course summary + with due dates, etc. - - grade_summary is the output from the course grader. More information on the format is in the docstring for CourseGrader. + - grade_summary is the output from the course grader. More information on + the format is in the docstring for CourseGrader. Arguments: student: A User object for the student to grade course: An XModule containing the course to grade - student_module_cache: A StudentModuleCache initialized with all instance_modules for the student + student_module_cache: A StudentModuleCache initialized with all + instance_modules for the student """ totaled_scores = {} chapters = [] @@ -51,12 +56,16 @@ def grade_sheet(student, course, grader, student_module_cache): correct = total if not total > 0: - #We simply cannot grade a problem that is 12/0, because we might need it as a percentage + #We simply cannot grade a problem that is 12/0, because we + #might need it as a percentage graded = False - scores.append(Score(correct, total, graded, module.metadata.get('display_name'))) + scores.append(Score(correct, total, graded, + module.metadata.get('display_name'))) + + section_total, graded_total = graders.aggregate_scores( + scores, s.metadata.get('display_name')) - section_total, graded_total = graders.aggregate_scores(scores, s.metadata.get('display_name')) #Add the graded total to totaled_scores format = s.metadata.get('format', "") if format and graded_total.possible > 0: @@ -65,7 +74,8 @@ def grade_sheet(student, course, grader, student_module_cache): totaled_scores[format] = format_scores sections.append({ - 'section': s.metadata.get('display_name'), + 'display_name': s.metadata.get('display_name'), + 'url_name': s.metadata.get('url_name'), 'scores': scores, 'section_total': section_total, 'format': format, @@ -74,7 +84,8 @@ def grade_sheet(student, course, grader, student_module_cache): }) chapters.append({'course': course.metadata.get('display_name'), - 'chapter': c.metadata.get('display_name'), + 'display_name': c.metadata.get('display_name'), + 'url_name': c.metadata.get('url_name'), 'sections': sections}) grade_summary = grader.grade(totaled_scores) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 91d4efa651..4699ed50a4 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -35,10 +35,12 @@ def toc_for_course(user, request, course, active_chapter, active_section): Create a table of contents from the module store Return format: - [ {'name': name, 'sections': SECTIONS, 'active': bool}, ... ] + [ {'display_name': name, 'url_name': url_name, + 'sections': SECTIONS, 'active': bool}, ... ] where SECTIONS is a list - [ {'name': name, 'format': format, 'due': due, 'active' : bool}, ...] + [ {'display_name': name, 'url_name': url_name, + 'format': format, 'due': due, 'active' : bool}, ...] active is set for the section and chapter corresponding to the passed parameters. Everything else comes from the xml, or defaults to "". @@ -59,12 +61,14 @@ def toc_for_course(user, request, course, active_chapter, active_section): hide_from_toc = section.metadata.get('hide_from_toc', 'false').lower() == 'true' if not hide_from_toc: - sections.append({'name': section.metadata.get('display_name'), + sections.append({'display_name': section.metadata.get('display_name'), + 'url_name': section.metadata.get('url_name'), 'format': section.metadata.get('format', ''), 'due': section.metadata.get('due', ''), 'active': active}) - chapters.append({'name': chapter.metadata.get('display_name'), + chapters.append({'display_name': chapter.metadata.get('display_name'), + 'url_name': chapter.metadata.get('url_name'), 'sections': sections, 'active': chapter.metadata.get('display_name') == active_chapter}) return chapters @@ -76,8 +80,8 @@ def get_section(course_module, chapter, section): or None if this doesn't specify a valid section course: Course url - chapter: Chapter name - section: Section name + chapter: Chapter url_name + section: Section url_name """ if course_module is None: @@ -85,7 +89,7 @@ def get_section(course_module, chapter, section): chapter_module = None for _chapter in course_module.get_children(): - if _chapter.metadata.get('display_name') == chapter: + if _chapter.metadata.get('url_name') == chapter: chapter_module = _chapter break @@ -94,7 +98,7 @@ def get_section(course_module, chapter, section): section_module = None for _section in chapter_module.get_children(): - if _section.metadata.get('display_name') == section: + if _section.metadata.get('url_name') == section: section_module = _section break @@ -141,12 +145,12 @@ def get_module(user, request, location, student_module_cache, position=None): # Setup system context for module instance ajax_url = settings.MITX_ROOT_URL + '/modx/' + descriptor.location.url() + '/' - # Fully qualified callback URL for external queueing system - xqueue_callback_url = (request.build_absolute_uri('/') + settings.MITX_ROOT_URL + - 'xqueue/' + str(user.id) + '/' + descriptor.location.url() + '/' + + # Fully qualified callback URL for external queueing system + xqueue_callback_url = (request.build_absolute_uri('/') + settings.MITX_ROOT_URL + + 'xqueue/' + str(user.id) + '/' + descriptor.location.url() + '/' + 'score_update') - # Default queuename is course-specific and is derived from the course that + # Default queuename is course-specific and is derived from the course that # contains the current module. # TODO: Queuename should be derived from 'course_settings.json' of each course xqueue_default_queuename = descriptor.location.org + '-' + descriptor.location.course diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 2b55b48caf..18b710e108 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -54,11 +54,6 @@ def user_groups(user): return group_names -def format_url_params(params): - return [urllib.quote(string.replace(' ', '_')) - if string is not None else None - for string in params] - @ensure_csrf_cookie @cache_if_anonymous @@ -124,7 +119,6 @@ def profile(request, course_id, student_id=None): 'language': user_info.language, 'email': student.email, 'course': course, - 'format_url_params': format_url_params, 'csrf': csrf(request)['csrf_token'] } context.update(grades.grade_sheet(student, course_module, course.grader, student_module_cache)) @@ -138,9 +132,9 @@ def render_accordion(request, course, chapter, section): If chapter and section are '' or None, renders a default accordion. - Returns (initialization_javascript, content)''' + Returns the html string''' - # TODO (cpennington): do the right thing with courses + # grab the table of contents toc = toc_for_course(request.user, request, course, chapter, section) active_chapter = 1 @@ -152,7 +146,6 @@ def render_accordion(request, course, chapter, section): ('toc', toc), ('course_name', course.title), ('course_id', course.id), - ('format_url_params', format_url_params), ('csrf', csrf(request)['csrf_token'])] + template_imports.items()) return render_to_string('accordion.html', context) @@ -169,9 +162,9 @@ def index(request, course_id, chapter=None, section=None, Arguments: - request : HTTP request - - course : coursename (str) - - chapter : chapter name (str) - - section : section name (str) + - course_id : course id (str: ORG/course/URL_NAME) + - chapter : chapter url_name (str) + - section : section url_name (str) - position : position in module, eg of module (str) Returns: @@ -180,16 +173,6 @@ def index(request, course_id, chapter=None, section=None, ''' course = check_course(course_id) - def clean(s): - ''' Fixes URLs -- we convert spaces to _ in URLs to prevent - funny encoding characters and keep the URLs readable. This undoes - that transformation. - ''' - return s.replace('_', ' ') if s is not None else None - - chapter = clean(chapter) - section = clean(section) - try: context = { 'csrf': csrf(request)['csrf_token'], @@ -202,8 +185,6 @@ def index(request, course_id, chapter=None, section=None, look_for_module = chapter is not None and section is not None if look_for_module: - # TODO (cpennington): Pass the right course in here - section_descriptor = get_section(course, chapter, section) if section_descriptor is not None: student_module_cache = StudentModuleCache(request.user, diff --git a/lms/templates/accordion.html b/lms/templates/accordion.html index defb424a29..353b83db70 100644 --- a/lms/templates/accordion.html +++ b/lms/templates/accordion.html @@ -1,13 +1,13 @@ <%! from django.core.urlresolvers import reverse %> <%def name="make_chapter(chapter)"> -

${chapter['name']}

+

${chapter['display_name']}

    % for section in chapter['sections']: - -

    ${section['name']} + +

    ${section['display_name']} ${section['format']} ${"due " + section['due'] if 'due' in section and section['due'] != '' else ''} diff --git a/lms/templates/profile.html b/lms/templates/profile.html index 6cda85fb03..8107bb1923 100644 --- a/lms/templates/profile.html +++ b/lms/templates/profile.html @@ -72,7 +72,7 @@ $(function() { var new_email = $('#new_email_field').val(); var new_password = $('#new_email_password').val(); - postJSON('/change_email',{"new_email":new_email, + postJSON('/change_email',{"new_email":new_email, "password":new_password}, function(data){ if(data.success){ @@ -81,7 +81,7 @@ $(function() { $("#change_email_error").html(data.error); } }); - log_event("profile", {"type":"email_change_request", + log_event("profile", {"type":"email_change_request", "old_email":"${email}", "new_email":new_email}); return false; @@ -91,7 +91,7 @@ $(function() { var new_name = $('#new_name_field').val(); var rationale = $('#name_rationale_field').val(); - postJSON('/change_name',{"new_name":new_name, + postJSON('/change_name',{"new_name":new_name, "rationale":rationale}, function(data){ if(data.success){ @@ -100,7 +100,7 @@ $(function() { $("#change_name_error").html(data.error); } }); - log_event("profile", {"type":"name_change_request", + log_event("profile", {"type":"name_change_request", "new_name":new_name, "rationale":rationale}); return false; @@ -125,9 +125,9 @@ $(function() {

      %for chapter in courseware_summary: - %if not chapter['chapter'] == "hidden": + %if not chapter['display_name'] == "hidden":
    1. -

      ${ chapter['chapter'] }

      +

      ${ chapter['display_name'] }

        %for section in chapter['sections']: @@ -137,14 +137,14 @@ $(function() { total = section['section_total'].possible percentageString = "{0:.0%}".format( float(earned)/total) if earned > 0 and total > 0 else "" %> - -

        - ${ section['section'] } ${"({0:.3n}/{1:.3n}) {2}".format( float(earned), float(total), percentageString )}

        + +

        + ${ section['display_name'] } ${"({0:.3n}/{1:.3n}) {2}".format( float(earned), float(total), percentageString )}

        ${section['format']} %if 'due' in section and section['due']!="": due ${section['due']} %endif - + %if len(section['scores']) > 0:
          ${ "Problem Scores: " if section['graded'] else "Practice Scores: "} @@ -153,7 +153,7 @@ $(function() { %endfor
        %endif - + %endfor
      @@ -181,7 +181,7 @@ $(function() {
    2. - Forum name: ${username} + Forum name: ${username}
    3. @@ -215,7 +215,7 @@ $(function() {
      -

      To uphold the credibility of edX certificates, name changes must go through an approval process. A member of the course staff will review your request, and if approved, update your information. Please allow up to a week for your request to be processed. Thank you.

      +

      To uphold the credibility of edX certificates, name changes must go through an approval process. A member of the course staff will review your request, and if approved, update your information. Please allow up to a week for your request to be processed. Thank you.

      • @@ -234,7 +234,7 @@ $(function() {
        -

        Change e-mail

        +

        Change e-mail

        From 2f911fd3b22be91d8f5a542e52f9ff1303d619dc Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 2 Aug 2012 14:08:28 -0400 Subject: [PATCH 19/22] add error-handling TODOs to capa_problem --- common/lib/capa/capa/capa_problem.py | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 93b6a085c1..cb3c19487d 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -288,20 +288,30 @@ class LoncapaProblem(object): try: ifp = self.system.filestore.open(file) # open using ModuleSystem OSFS filestore except Exception as err: - log.error('Error %s in problem xml include: %s' % (err, etree.tostring(inc, pretty_print=True))) - log.error('Cannot find file %s in %s' % (file, self.system.filestore)) - if not self.system.get('DEBUG'): # if debugging, don't fail - just log error + log.error('Error %s in problem xml include: %s' % ( + err, etree.tostring(inc, pretty_print=True))) + log.error('Cannot find file %s in %s' % ( + file, self.system.filestore)) + # if debugging, don't fail - just log error + # TODO (vshnayder): need real error handling, display to users + if not self.system.get('DEBUG'): raise - else: continue + else: + continue try: incxml = etree.XML(ifp.read()) # read in and convert to XML except Exception as err: - log.error('Error %s in problem xml include: %s' % (err, etree.tostring(inc, pretty_print=True))) + log.error('Error %s in problem xml include: %s' % ( + err, etree.tostring(inc, pretty_print=True))) log.error('Cannot parse XML in %s' % (file)) - if not self.system.get('DEBUG'): # if debugging, don't fail - just log error + # if debugging, don't fail - just log error + # TODO (vshnayder): same as above + if not self.system.get('DEBUG'): raise - else: continue - parent = inc.getparent() # insert new XML into tree in place of inlcude + else: + continue + # insert new XML into tree in place of inlcude + parent = inc.getparent() parent.insert(parent.index(inc), incxml) parent.remove(inc) log.debug('Included %s into %s' % (file, self.problem_id)) From b46172da9b00d224a327da64b2168e648b4a0717 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 3 Aug 2012 11:36:54 -0400 Subject: [PATCH 20/22] Rename module.name and descriptor.name to url_name * update templates and code references * also a display_name property that defaults to cleaned url_name --- cms/djangoapps/contentstore/views.py | 2 +- cms/templates/unit.html | 2 +- cms/templates/widgets/navigation.html | 4 ++-- cms/templates/widgets/sequence-edit.html | 2 +- common/lib/xmodule/xmodule/html_module.py | 4 ++-- .../lib/xmodule/xmodule/modulestore/search.py | 4 ++-- common/lib/xmodule/xmodule/video_module.py | 9 ++++---- common/lib/xmodule/xmodule/x_module.py | 22 +++++++++++++++++-- common/lib/xmodule/xmodule/xml_module.py | 6 ++--- lms/djangoapps/courseware/courses.py | 2 +- lms/templates/video.html | 2 +- 11 files changed, 39 insertions(+), 20 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 0447254cfb..0305795e52 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -108,7 +108,7 @@ def edit_item(request): 'contents': item.get_html(), 'js_module': item.js_module_name, 'category': item.category, - 'name': item.name, + 'url_name': item.url_name, 'previews': get_module_previews(request, item), }) diff --git a/cms/templates/unit.html b/cms/templates/unit.html index 6aa780d42a..828f95ed47 100644 --- a/cms/templates/unit.html +++ b/cms/templates/unit.html @@ -1,7 +1,7 @@
        -

        ${name}

        +

        ${url_name}

        ${category}

        diff --git a/cms/templates/widgets/navigation.html b/cms/templates/widgets/navigation.html index 9f9b37d571..ce18e867bd 100644 --- a/cms/templates/widgets/navigation.html +++ b/cms/templates/widgets/navigation.html @@ -41,7 +41,7 @@ % for week in weeks:
      • -

        ${week.name}

        +

        ${week.url_name}

          % if 'goals' in week.metadata: % for goal in week.metadata['goals']: @@ -60,7 +60,7 @@ data-type="${module.js_module_name}" data-preview-type="${module.module_class.js_module_name}"> - ${module.name} + ${module.url_name} handle % endfor diff --git a/cms/templates/widgets/sequence-edit.html b/cms/templates/widgets/sequence-edit.html index f7108e366e..c623eb4ec2 100644 --- a/cms/templates/widgets/sequence-edit.html +++ b/cms/templates/widgets/sequence-edit.html @@ -39,7 +39,7 @@ ${child.name} + data-preview-type="${child.module_class.js_module_name}">${child.url_name} handle %endfor diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 7c3456e5ad..7a09004e33 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -121,13 +121,13 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # Not proper format. Write html to file, return an empty tag filepath = u'{category}/{name}.html'.format(category=self.category, - name=self.name) + name=self.url_name) resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) with resource_fs.open(filepath, 'w') as file: file.write(self.definition['data']) elt = etree.Element('html') - elt.set("filename", self.name) + elt.set("filename", self.url_name) return elt diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index a383b3f8ec..b3eae64fcc 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -87,8 +87,8 @@ def path_to_location(modulestore, location, course_name=None): n = len(path) course_id = CourseDescriptor.location_to_id(path[0]) - chapter = path[1].name if n > 1 else None - section = path[2].name if n > 2 else None + chapter = path[1].url_name if n > 1 else None + section = path[2].url_name if n > 2 else None # TODO (vshnayder): not handling position at all yet... position = None diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index da10e4bc91..fb68ba982b 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -23,11 +23,12 @@ class VideoModule(XModule): css = {'scss': [resource_string(__name__, 'css/video/display.scss')]} js_module_name = "Video" - def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs) + def __init__(self, system, location, definition, + instance_state=None, shared_state=None, **kwargs): + XModule.__init__(self, system, location, definition, + instance_state, shared_state, **kwargs) xmltree = etree.fromstring(self.definition['data']) self.youtube = xmltree.get('youtube') - self.name = xmltree.get('name') self.position = 0 if instance_state is not None: @@ -71,7 +72,7 @@ class VideoModule(XModule): 'streams': self.video_list(), 'id': self.location.html_id(), 'position': self.position, - 'name': self.name, + 'display_name': self.display_name, # TODO (cpennington): This won't work when we move to data that isn't on the filesystem 'data_dir': self.metadata['data_dir'], }) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 1d16849d67..f6a43f2612 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -191,11 +191,20 @@ class XModule(HTMLSnippet): self.instance_state = instance_state self.shared_state = shared_state self.id = self.location.url() - self.name = self.location.name + self.url_name = self.location.name self.category = self.location.category self.metadata = kwargs.get('metadata', {}) self._loaded_children = None + @property + def display_name(self): + ''' + Return a display name for the module: use display_name if defined in + metadata, otherwise convert the url name. + ''' + return self.metadata.get('display_name', + self.url_name.replace('_', ' ')) + def get_children(self): ''' Return module instances for all the children of this module. @@ -355,13 +364,22 @@ class XModuleDescriptor(Plugin, HTMLSnippet): self.metadata = kwargs.get('metadata', {}) self.definition = definition if definition is not None else {} self.location = Location(kwargs.get('location')) - self.name = self.location.name + self.url_name = self.location.name self.category = self.location.category self.shared_state_key = kwargs.get('shared_state_key') self._child_instances = None self._inherited_metadata = set() + @property + def display_name(self): + ''' + Return a display name for the module: use display_name if defined in + metadata, otherwise convert the url name. + ''' + return self.metadata.get('display_name', + self.url_name.replace('_', ' ')) + @property def own_metadata(self): """ diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index ea13bc2640..b0a289d149 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -225,7 +225,7 @@ class XmlDescriptor(XModuleDescriptor): # Write it to a file if necessary if self.split_to_file(xml_object): # Put this object in its own file - filepath = self.__class__._format_filepath(self.category, self.name) + filepath = self.__class__._format_filepath(self.category, self.url_name) resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) with resource_fs.open(filepath, 'w') as file: file.write(etree.tostring(xml_object, pretty_print=True)) @@ -238,10 +238,10 @@ class XmlDescriptor(XModuleDescriptor): xml_object.tail = '' - xml_object.set('filename', self.name) + xml_object.set('filename', self.url_name) # Add the metadata - xml_object.set('url_name', self.name) + xml_object.set('url_name', self.url_name) for attr in self.metadata_attributes: attr_map = self.xml_attribute_map.get(attr, AttrMap(attr)) metadata_key = attr_map.metadata_key diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index cfda6497d5..19eef3ee80 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -83,7 +83,7 @@ def get_course_about_section(course, section_key): log.warning("Missing about section {key} in course {url}".format(key=section_key, url=course.location.url())) return None elif section_key == "title": - return course.metadata.get('display_name', course.name) + return course.metadata.get('display_name', course.url_name) elif section_key == "university": return course.location.org elif section_key == "number": diff --git a/lms/templates/video.html b/lms/templates/video.html index 65ff44e8fa..93273ddb87 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -1,5 +1,5 @@ % if name is not UNDEFINED and name is not None: -

          ${name}

          +

          ${display_name}

          % endif
          From 0b2069c61959d9509703c84d503d3dc6c551ea90 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 2 Aug 2012 15:14:13 -0400 Subject: [PATCH 21/22] make clean_xml script work with stringified errors --- .../courseware/management/commands/clean_xml.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/courseware/management/commands/clean_xml.py b/lms/djangoapps/courseware/management/commands/clean_xml.py index 1773c3d21f..de845df572 100644 --- a/lms/djangoapps/courseware/management/commands/clean_xml.py +++ b/lms/djangoapps/courseware/management/commands/clean_xml.py @@ -61,11 +61,7 @@ def import_with_checks(course_dir, verbose=True): course_dirs=course_dirs) def str_of_err(tpl): - (msg, exc_info) = tpl - if exc_info is None: - return msg - - exc_str = '\n'.join(traceback.format_exception(*exc_info)) + (msg, exc_str) = tpl return '{msg}\n{exc}'.format(msg=msg, exc=exc_str) courses = modulestore.get_courses() @@ -83,7 +79,7 @@ def import_with_checks(course_dir, verbose=True): print '\n' print "=" * 40 print 'ERRORs during import:' - print '\n'.join(map(str_of_err,errors)) + print '\n'.join(map(str_of_err, errors)) print "=" * 40 print '\n' From 94e24c16260b85b672cebf9bdfa14b7e86d3bf7c Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 3 Aug 2012 14:45:43 -0400 Subject: [PATCH 22/22] Leftover name->url_name fixes --- .../lib/xmodule/xmodule/modulestore/search.py | 5 +++-- lms/djangoapps/courseware/grades.py | 10 +++++----- lms/djangoapps/courseware/module_render.py | 18 +++++++++--------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index b3eae64fcc..baf3d46b57 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -87,8 +87,9 @@ def path_to_location(modulestore, location, course_name=None): n = len(path) course_id = CourseDescriptor.location_to_id(path[0]) - chapter = path[1].url_name if n > 1 else None - section = path[2].url_name if n > 2 else None + # pull out the location names + chapter = path[1].name if n > 1 else None + section = path[2].name if n > 2 else None # TODO (vshnayder): not handling position at all yet... position = None diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 717cbde140..6e7a0ce102 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -74,8 +74,8 @@ def grade_sheet(student, course, grader, student_module_cache): totaled_scores[format] = format_scores sections.append({ - 'display_name': s.metadata.get('display_name'), - 'url_name': s.metadata.get('url_name'), + 'display_name': s.display_name, + 'url_name': s.url_name, 'scores': scores, 'section_total': section_total, 'format': format, @@ -83,9 +83,9 @@ def grade_sheet(student, course, grader, student_module_cache): 'graded': graded, }) - chapters.append({'course': course.metadata.get('display_name'), - 'display_name': c.metadata.get('display_name'), - 'url_name': c.metadata.get('url_name'), + chapters.append({'course': course.display_name, + 'display_name': c.display_name, + 'url_name': c.url_name, 'sections': sections}) grade_summary = grader.grade(totaled_scores) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 4699ed50a4..9260e15c61 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -56,21 +56,21 @@ def toc_for_course(user, request, course, active_chapter, active_section): sections = list() for section in chapter.get_display_items(): - active = (chapter.metadata.get('display_name') == active_chapter and - section.metadata.get('display_name') == active_section) + active = (chapter.display_name == active_chapter and + section.display_name == active_section) hide_from_toc = section.metadata.get('hide_from_toc', 'false').lower() == 'true' if not hide_from_toc: - sections.append({'display_name': section.metadata.get('display_name'), - 'url_name': section.metadata.get('url_name'), + sections.append({'display_name': section.display_name, + 'url_name': section.url_name, 'format': section.metadata.get('format', ''), 'due': section.metadata.get('due', ''), 'active': active}) - chapters.append({'display_name': chapter.metadata.get('display_name'), - 'url_name': chapter.metadata.get('url_name'), + chapters.append({'display_name': chapter.display_name, + 'url_name': chapter.url_name, 'sections': sections, - 'active': chapter.metadata.get('display_name') == active_chapter}) + 'active': chapter.display_name == active_chapter}) return chapters @@ -89,7 +89,7 @@ def get_section(course_module, chapter, section): chapter_module = None for _chapter in course_module.get_children(): - if _chapter.metadata.get('url_name') == chapter: + if _chapter.url_name == chapter: chapter_module = _chapter break @@ -98,7 +98,7 @@ def get_section(course_module, chapter, section): section_module = None for _section in chapter_module.get_children(): - if _section.metadata.get('url_name') == section: + if _section.url_name == section: section_module = _section break