Merge pull request #182 from MITx/victor/cms_more_auth

Victor/cms more auth

cms/djangoapps/contentstore/tests/tests.py

@@ -4,45 +4,165 @@ from django.test import TestCase
 from mock import patch, Mock
 from override_settings import override_settings
 from django.conf import settings
+from django.core.urlresolvers import reverse
+
+from student.models import Registration
+from django.contrib.auth.models import User
+
 
 def parse_json(response):
     """Parse response, which is assumed to be json"""
     return json.loads(response.content)
 
-class AuthTestCase(TestCase):
-    """Check that various permissions-related things work"""
 
-    def test_index(self):
-        """Make sure the main page loads."""
-        resp = self.client.get('/')
-        self.assertEqual(resp.status_code, 200)
+def user(email):
+    '''look up a user by email'''
+    return User.objects.get(email=email)
 
-    def test_signup_load(self):
-        """Make sure the signup page loads."""
-        resp = self.client.get('/signup')
-        self.assertEqual(resp.status_code, 200)
+def registration(email):
+    '''look up registration object by email'''
+    return Registration.objects.get(user__email=email)
 
+class AuthTestCase(TestCase):
+    """Check that various permissions-related things work"""
 
-    def test_create_account(self):
+    def setUp(self):
+        self.email = 'a@b.com'
+        self.pw = 'xyz'
+        self.username = 'testuser'
+
+    def check_page_get(self, url, expected):
+        resp = self.client.get(url)
+        self.assertEqual(resp.status_code, expected)
+        return resp
+        
+    def test_public_pages_load(self):
+        """Make sure pages that don't require login load without error."""
+        pages = (
+                 reverse('login'),
+                 reverse('signup'),
+                 )
+        for page in pages:
+            print "Checking '{0}'".format(page)
+            self.check_page_get(page, 200)
 
+    def test_create_account_errors(self):
         # No post data -- should fail
         resp = self.client.post('/create_account', {})
         self.assertEqual(resp.status_code, 200)
         data = parse_json(resp)
         self.assertEqual(data['success'], False)
-        
-        # Should work
+
+    def _create_account(self, username, email, pw):
+        '''Try to create an account.  No error checking'''
         resp = self.client.post('/create_account', {
-            'username': 'user',
-            'email': 'a@b.com',
-            'password': 'xyz',
+            'username': username,
+            'email': email,
+            'password': pw,
             'location' : 'home',
             'language' : 'Franglish',
             'name' : 'Fred Weasley',
             'terms_of_service' : 'true',
             'honor_code' : 'true'})
+        return resp
+
+    def create_account(self, username, email, pw):
+        '''Create the account and check that it worked'''
+        resp = self._create_account(username, email, pw)
         self.assertEqual(resp.status_code, 200)
         data = parse_json(resp)
         self.assertEqual(data['success'], True)
+
+        # Check both that the user is created, and inactive
+        self.assertFalse(user(self.email).is_active)
+
+        return resp
+
+    def _activate_user(self, email):
+        '''look up the user's activation key in the db, then hit the activate view.
+        No error checking'''
+        activation_key = registration(email).activation_key
+
+        # and now we try to activate
+        resp = self.client.get(reverse('activate', kwargs={'key': activation_key}))
+        return resp
+
+    def activate_user(self, email):
+        resp = self._activate_user(email)
+        self.assertEqual(resp.status_code, 200)
+        # Now make sure that the user is now actually activated
+        self.assertTrue(user(self.email).is_active)
+
+    def test_create_account(self):
+        self.create_account(self.username, self.email, self.pw)
+        self.activate_user(self.email)
+
+
+    def _login(self, email, pw):
+        '''Login.  View should always return 200.  The success/fail is in the
+        returned json'''
+        resp = self.client.post(reverse('login_post'),
+                                {'email': email, 'password': pw})
+        self.assertEqual(resp.status_code, 200)
+        return resp
+
+
+    def login(self, email, pw):
+        '''Login, check that it worked.'''
+        resp = self._login(self.email, self.pw)
+        data = parse_json(resp)
+        self.assertTrue(data['success'])
+        return resp
+
+    def test_login(self):
+        self.create_account(self.username, self.email, self.pw)
+
+        # Not activated yet.  Login should fail.
+        resp = self._login(self.email, self.pw)
+        data = parse_json(resp)
+        self.assertFalse(data['success'])
         
+        self.activate_user(self.email)
+
+        # Now login should work
+        self.login(self.email, self.pw)
+
+    def test_private_pages_auth(self):
+        """Make sure pages that do require login work."""
+        auth_pages = (
+            reverse('index'),
+            reverse('edit_item'),
+            reverse('save_item'),
+            )
+
+        # These are pages that should just load when the user is logged in
+        # (no data needed)
+        simple_auth_pages = (
+            reverse('index'),
+            )
+
+        # need an activated user
+        self.test_create_account()
+
+        # Not logged in.  Should redirect to login.
+        print 'Not logged in'
+        for page in auth_pages:
+            print "Checking '{0}'".format(page)
+            self.check_page_get(page, expected=302)
+
+        # Logged in should work.
+        self.login(self.email, self.pw)
+
+        print 'Logged in'
+        for page in simple_auth_pages:
+            print "Checking '{0}'".format(page)
+            self.check_page_get(page, expected=200)
         
+
+    def test_index_auth(self):
+
+        # not logged in.  Should return a redirect.
+        resp = self.client.get(reverse('index'))
+        self.assertEqual(resp.status_code, 302)
+
+        # Logged in should work.

cms/djangoapps/contentstore/views.py

@@ -2,6 +2,7 @@ from util.json_request import expect_json
 import json
 
 from django.http import HttpResponse
+from django.contrib.auth.decorators import login_required
 from django.core.context_processors import csrf
 from django_future.csrf import ensure_csrf_cookie
 from django.core.urlresolvers import reverse
@@ -13,8 +14,28 @@ from github_sync import export_to_github
 from mitxmako.shortcuts import render_to_response
 from xmodule.modulestore.django import modulestore
 
+# ==== Public views ==================================================
 
 @ensure_csrf_cookie
+def signup(request):
+    """
+    Display the signup form.
+    """
+    csrf_token = csrf(request)['csrf_token']
+    return render_to_response('signup.html', {'csrf': csrf_token }) 
+
+@ensure_csrf_cookie
+def login_page(request):
+    """
+    Display the login form.
+    """
+    csrf_token = csrf(request)['csrf_token']
+    return render_to_response('login.html', {'csrf': csrf_token }) 
+    
+# ==== Views for any logged-in user ==================================
+
+@login_required
+@ensure_csrf_cookie
 def index(request):
     courses = modulestore().get_items(['i4x', None, None, 'course', None])
     return render_to_response('index.html', {
@@ -26,25 +47,32 @@ def index(request):
                     for course in courses]
     })
 
+# ==== Views with per-item permissions================================
 
-@ensure_csrf_cookie
-def signup(request):
-    """
-    Display the signup form.
-    """
-    csrf_token = csrf(request)['csrf_token']
-    return render_to_response('signup.html', {'csrf': csrf_token }) 
+def has_access(user, location):
+    '''Return True if user allowed to access this piece of data'''
+    # TODO (vshnayder): actually check perms
+    return user.is_active and user.is_authenticated
 
+@login_required
 @ensure_csrf_cookie
 def course_index(request, org, course, name):
+    location = ['i4x', org, course, 'course', name]
+    if not has_access(request.user, location):
+        raise Http404  # TODO (vshnayder): better error
+    
     # TODO (cpennington): These need to be read in from the active user
-    course = modulestore().get_item(['i4x', org, course, 'course', name])
+    course = modulestore().get_item(location)
     weeks = course.get_children()
     return render_to_response('course_index.html', {'weeks': weeks})
 
-
+@login_required
 def edit_item(request):
+    # TODO (vshnayder): Why are we using "id" instead of "location"?
     item_id = request.GET['id']
+    if not has_access(request.user, item_id):
+        raise Http404  # TODO (vshnayder): better error
+        
     item = modulestore().get_item(item_id)
     return render_to_response('unit.html', {
         'contents': item.get_html(),
@@ -54,9 +82,28 @@ def edit_item(request):
     })
 
 
+def user_author_string(user):
+    '''Get an author string for commits by this user.  Format:
+    first last <email@email.com>.
+
+    If the first and last names are blank, uses the username instead.
+    Assumes that the email is not blank.
+    '''
+    f = user.first_name
+    l = user.last_name
+    if f == '' and l == '':
+        f = user.username
+    return '{first} {last} <{email}>'.format(first=f,
+                                             last=l,
+                                             email=user.email)
+
+@login_required
 @expect_json
 def save_item(request):
     item_id = request.POST['id']
+    if not has_access(request.user, item_id):
+        raise Http404  # TODO (vshnayder): better error
+
     data = json.loads(request.POST['data'])
     modulestore().update_item(item_id, data)
 
@@ -66,6 +113,7 @@ def save_item(request):
     course_location = Location(item_id)._replace(category='course', name=None)
     courses = modulestore().get_items(course_location, depth=None)
     for course in courses:
-        export_to_github(course, "CMS Edit")
+        author_string = user_author_string(request.user)
+        export_to_github(course, "CMS Edit", author_string)
 
     return HttpResponse(json.dumps({}))

cms/djangoapps/github_sync/__init__.py

@@ -38,7 +38,12 @@ def import_from_github(repo_settings):
     return git_repo.head.commit.hexsha, module_store.courses[course_dir]
 
 
-def export_to_github(course, commit_message):
+def export_to_github(course, commit_message, author_str=None):
+    '''
+    Commit any changes to the specified course with given commit message,
+    and push to github (if MITX_FEATURES['GITHUB_PUSH'] is True).
+    If author_str is specified, uses it in the commit.
+    '''
     repo_path = settings.DATA_DIR / course.metadata.get('course_dir', course.location.course)
     fs = OSFS(repo_path)
     xml = course.export_to_xml(fs)
@@ -49,8 +54,11 @@ def export_to_github(course, commit_message):
     git_repo = Repo(repo_path)
     if git_repo.is_dirty():
         git_repo.git.add(A=True)
-        git_repo.git.commit(m=commit_message)
-
+        if author_str is not None:
+            git_repo.git.commit(m=commit_message, author=author_str)
+        else:
+            git_repo.git.commit(m=commit_message)
+        
         origin = git_repo.remotes.origin
         if settings.MITX_FEATURES['GITHUB_PUSH']:
             push_infos = origin.push()

cms/djangoapps/github_sync/views.py

@@ -49,4 +49,4 @@ def github_post_receive(request):
     revision, course = import_from_github(repo)
     export_to_github(course, repo['path'], "Changes from cms import of revision %s" % revision)
 
-    return HttpResponse('Push recieved')
+    return HttpResponse('Push received')

cms/envs/common.py

@@ -70,6 +70,10 @@ TEMPLATE_DIRS = (
 
 MITX_ROOT_URL = ''
 
+LOGIN_REDIRECT_URL = MITX_ROOT_URL + '/login'
+LOGIN_URL = MITX_ROOT_URL + '/login'
+
+
 TEMPLATE_CONTEXT_PROCESSORS = (
     'django.core.context_processors.request',
     'django.core.context_processors.static',

cms/envs/dev.py

@@ -29,39 +29,37 @@ DATABASES = {
     }
 }
 
-REPO_ROOT = ENV_ROOT / "content"
-
 REPOS = {
     'edx4edx': {
-        'path': REPO_ROOT / "edx4edx",
+        'path': DATA_DIR / "edx4edx",
         'org': 'edx',
         'course': 'edx4edx',
         'branch': 'for_cms',
         'origin': 'git@github.com:MITx/edx4edx.git',
     },
     '6002x-fall-2012': {
-        'path': REPO_ROOT / '6002x-fall-2012',
+        'path': DATA_DIR / '6002x-fall-2012',
         'org': 'mit.edu',
         'course': '6.002x',
         'branch': 'for_cms',
         'origin': 'git@github.com:MITx/6002x-fall-2012.git',
     },
     '6.00x': {
-        'path': REPO_ROOT / '6.00x',
+        'path': DATA_DIR / '6.00x',
         'org': 'mit.edu',
         'course': '6.00x',
         'branch': 'for_cms',
         'origin': 'git@github.com:MITx/6.00x.git',
     },
     '7.00x': {
-        'path': REPO_ROOT / '7.00x',
+        'path': DATA_DIR / '7.00x',
         'org': 'mit.edu',
         'course': '7.00x',
         'branch': 'for_cms',
         'origin': 'git@github.com:MITx/7.00x.git',
     },
     '3.091x': {
-        'path': REPO_ROOT / '3.091x',
+        'path': DATA_DIR / '3.091x',
         'org': 'mit.edu',
         'course': '3.091x',
         'branch': 'for_cms',

cms/templates/activation_active.html

-<%inherit file="marketing.html" />
+<%inherit file="base.html" />
 
 <%block name="content">
 
@@ -7,8 +7,7 @@
 
 <section class="activation">
   <h1>Account already active!</h1>
-  <p> This account has already been activated. You can log in at
-  the <a href="/">home page</a>.</p>
+  <p> This account has already been activated. <a href="/login">Log in here</a>.</p>
 </div>
 </section>
 

cms/templates/activation_complete.html

-<%inherit file="marketing.html" />
-
+<%inherit file="base.html" />
 
 <%block name="content">
 
 <section class="tos">
 <div>
   <h1>Activation Complete!</h1>
-  <p>Thanks for activating your account. You can log in at the <a href="/">home page</a>.</p>
+  <p>Thanks for activating your account. <a href="/login">Log in here</a>.</p>
 </div>
 </section>
 

cms/templates/activation_invalid.html

-<%inherit file="marketing.html" />
+<%inherit file="base.html" />
 
 <%block name="content">
 <section class="tos">

cms/templates/login.html

-<form name="login" action="login", method="post">
-    <input type="hidden" name="csrfmiddlewaretoken" value="${csrf_token}"/>
+<%inherit file="base.html" />
+<%! from django.core.urlresolvers import reverse %>
+<%block name="title">Log in</%block>
 
-    % if next is not None:
-    <input type="hidden" name="next" value="${next}"/>
-    % endif
+<%block name="content">
 
-    Username: <input type="text" name="username" />
-    Possword: <input type="password" name="password" />
-    <input type="submit" value="Submit" />
-</form>
+<section class="main-container">
+
+  <section class="main-content">
+    <header>
+      <h3>Log in</h3>
+      <hr>
+    </header>
+
+    <form id="login_form" action="login_post" method="post">
+      <label>E-mail</label>
+      <input name="email" type="email" placeholder="E-mail">
+      <label>Password</label>
+      <input name="password" type="password" placeholder="Password">
+      <label class="remember-me">
+        <input name="remember" type="checkbox">
+        Remember me
+      </label>
+      <div class="submit">
+        <input name="submit" type="submit" value="Submit">
+      </div>
+    </form>
+
+    <section class="login-extra">
+      <p>
+        <span>Not enrolled? <a href="${reverse('signup')}">Sign up.</a></span>
+        <a href="#" class="pwd-reset">Forgot password?</a>
+      </p>
+    </section>
+
+
+  </section>
+
+</section>
+
+<script type="text/javascript">
+(function() {
+   function getCookie(name) {
+     return $.cookie(name);
+   }
+   function postJSON(url, data, callback) {
+     $.ajax({type:'POST',
+       url: url,
+       dataType: 'json',
+       data: data,
+       success: callback,
+       headers : {'X-CSRFToken':getCookie('csrftoken')}
+     });
+   }
+
+   $('form#login_form').submit(function(e) {
+     e.preventDefault();
+     var submit_data = $('#login_form').serialize();
+
+     postJSON('/login_post',
+       submit_data,
+       function(json) {
+       if(json.success) {
+        location.href="${reverse('index')}";
+        } else if($('#login_error').length == 0) {
+        $('#login_form').prepend('<div id="login_error">Email or password is incorrect.</div>');
+        } else {
+        $('#login_error').stop().css("background-color", "#933").animate({ backgroundColor: "#333"}, 2000);
+        }
+        }
+        );
+     });
+})(this)
+</script>
+      
+</%block>

cms/templates/marketing.html

-<%inherit file="base.html" />
\ No newline at end of file

cms/templates/registration/reg_complete.html

+<h1>Check your email</h1>
+<p>An activation link has been sent to ${ email }, along with
+instructions for activating your account.</p>

cms/templates/widgets/header.html

+<%! from django.core.urlresolvers import reverse %>
 <header>
   <nav>
-    <h2><a href="/">6.002x circuits and electronics</a></h2>
+    <h2><a href="/">edX CMS: TODO:-course-name-here</a></h2>
     <ul>
       <li>
         <a href="#" class="new-module wip">New Module</a>
@@ -13,6 +14,12 @@
     <ul class="user-nav">
       <li><a href="#" class="wip">Tasks</a></li>
       <li><a href="#" class="wip">Settings</a></li>
+      % if user.is_authenticated():
+         <li><a href="${reverse('logout')}">Log out</a></li>
+      % else:
+         <li><a href="${reverse('login')}">Log in</a></li>
+      % endif
+
     </ul>
   </nav>
 </header>

cms/urls.py

@@ -11,16 +11,25 @@ urlpatterns = ('',
     url(r'^$', 'contentstore.views.index', name='index'),
     url(r'^edit_item$', 'contentstore.views.edit_item', name='edit_item'),
     url(r'^save_item$', 'contentstore.views.save_item', name='save_item'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)$', 'contentstore.views.course_index', name='course_index'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)$',
+        'contentstore.views.course_index', name='course_index'),
     url(r'^github_service_hook$', 'github_sync.views.github_post_receive'),
 )
 
 # User creation and updating views
 urlpatterns += (
-    url(r'^signup$', 'contentstore.views.signup'),
+    url(r'^signup$', 'contentstore.views.signup', name='signup'),
 
     url(r'^create_account$', 'student.views.create_account'),
-    url(r'^activate/(?P<key>[^/]*)$', 'student.views.activate_account'),
+    url(r'^activate/(?P<key>[^/]*)$', 'student.views.activate_account', name='activate'),
+
+    # form page
+    url(r'^login$', 'contentstore.views.login_page', name='login'),
+    # ajax view that actually does the work
+    url(r'^login_post$', 'student.views.login_user', name='login_post'),
+
+    url(r'^logout$', 'student.views.logout_user', name='logout'),
+
     )
 
 if settings.DEBUG:

common/djangoapps/student/views.py

@@ -146,12 +146,12 @@ def create_account(request, post_override=None):
             js['value'] = "Error (401 {field}). E-mail us.".format(field=a)
             return HttpResponse(json.dumps(js))
 
-    if 'honor_code' not in post_vars or post_vars['honor_code'] != u'true':
+    if post_vars.get('honor_code', 'false') != u'true':
         js['value']="To enroll, you must follow the honor code.".format(field=a)
         return HttpResponse(json.dumps(js))
 
 
-    if 'terms_of_service' not in post_vars or post_vars['terms_of_service'] != u'true':
+    if post_vars.get('terms_of_service', 'false') != u'true':
         js['value']="You must accept the terms of service.".format(field=a)
         return HttpResponse(json.dumps(js))
 

doc/overview.md

@@ -108,7 +108,7 @@ environments, defined in `cms/envs`.
 
 - javascript -- we use coffeescript, which compiles to js, and is much nicer to work with.  Look for `*.coffee` files.  We use _jasmine_ for testing js.
 
-- _mako_  -- we use this for templates, and have a fork called mitxmako (TODO: why did we have to fork mako?)
+- _mako_  -- we use this for templates, and have wrapper called mitxmako that makes mako look like the django templating calls.
 
 We use a fork of django-pipeline to make sure that the js and css always reflect the latest `*.coffee` and `*.sass` files (We're hoping to get our changes merged in the official version soon).  This works differently in development and production.  Test uses the production settings.  
 

rakefile

@@ -88,9 +88,8 @@ end
 
     # Per System tasks
     desc "Run all django tests on our djangoapps for the #{system}"
-    task "test_#{system}" => [report_dir, :predjango, "#{system}:collectstatic:test"] do
-        run_tests(system, report_dir)
-    end
+  task "test_#{system}" => ["#{system}:collectstatic:test", "fasttest_#{system}"]
+
     # Have a way to run the tests without running collectstatic -- useful when debugging without
     # messing with static files.
     task "fasttest_#{system}" => [report_dir, :predjango] do