From f8f2e2121ac1d9c77a66250f9d7dd4583049710d Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Thu, 11 Jun 2015 21:06:02 -0400 Subject: [PATCH] Revert "[LTI Provider] Basic LTI authentication" --- .../migrations/0003_create_lti_user_model.py | 108 ------------ lms/djangoapps/lti_provider/models.py | 18 -- .../lti_provider/tests/test_users.py | 157 ------------------ .../lti_provider/tests/test_views.py | 39 ++--- lms/djangoapps/lti_provider/users.py | 94 ----------- lms/djangoapps/lti_provider/views.py | 19 +-- 6 files changed, 21 insertions(+), 414 deletions(-) delete mode 100644 lms/djangoapps/lti_provider/migrations/0003_create_lti_user_model.py delete mode 100644 lms/djangoapps/lti_provider/tests/test_users.py delete mode 100644 lms/djangoapps/lti_provider/users.py diff --git a/lms/djangoapps/lti_provider/migrations/0003_create_lti_user_model.py b/lms/djangoapps/lti_provider/migrations/0003_create_lti_user_model.py deleted file mode 100644 index 22caf03f6f..0000000000 --- a/lms/djangoapps/lti_provider/migrations/0003_create_lti_user_model.py +++ /dev/null @@ -1,108 +0,0 @@ -# -*- coding: utf-8 -*- -# pylint: disable=invalid-name, missing-docstring, unused-argument, unused-import, line-too-long -from south.utils import datetime_utils as datetime -from south.db import db -from south.v2 import SchemaMigration -from django.db import models - - -class Migration(SchemaMigration): - - def forwards(self, orm): - # Adding model 'LtiUser' - db.create_table('lti_provider_ltiuser', ( - ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), - ('lti_consumer', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['lti_provider.LtiConsumer'])), - ('lti_user_id', self.gf('django.db.models.fields.CharField')(max_length=255)), - ('edx_user', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], unique=True)), - )) - db.send_create_signal('lti_provider', ['LtiUser']) - - # Adding unique constraint on 'LtiUser', fields ['lti_consumer', 'lti_user_id'] - db.create_unique('lti_provider_ltiuser', ['lti_consumer_id', 'lti_user_id']) - - # Adding unique constraint on 'LtiConsumer', fields ['instance_guid'] - db.create_unique('lti_provider_lticonsumer', ['instance_guid']) - - - def backwards(self, orm): - # Removing unique constraint on 'LtiConsumer', fields ['instance_guid'] - db.delete_unique('lti_provider_lticonsumer', ['instance_guid']) - - # Removing unique constraint on 'LtiUser', fields ['lti_consumer', 'lti_user_id'] - db.delete_unique('lti_provider_ltiuser', ['lti_consumer_id', 'lti_user_id']) - - # Deleting model 'LtiUser' - db.delete_table('lti_provider_ltiuser') - - - models = { - 'auth.group': { - 'Meta': {'object_name': 'Group'}, - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), - 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) - }, - 'auth.permission': { - 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, - 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), - 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) - }, - 'auth.user': { - 'Meta': {'object_name': 'User'}, - 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), - 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), - 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), - 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), - 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), - 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), - 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), - 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), - 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), - 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), - 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) - }, - 'contenttypes.contenttype': { - 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, - 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), - 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) - }, - 'lti_provider.gradedassignment': { - 'Meta': {'unique_together': "(('outcome_service', 'lis_result_sourcedid'),)", 'object_name': 'GradedAssignment'}, - 'course_key': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}), - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'lis_result_sourcedid': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), - 'outcome_service': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['lti_provider.OutcomeService']"}), - 'usage_key': ('xmodule_django.models.UsageKeyField', [], {'max_length': '255', 'db_index': 'True'}), - 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) - }, - 'lti_provider.lticonsumer': { - 'Meta': {'object_name': 'LtiConsumer'}, - 'consumer_key': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32', 'db_index': 'True'}), - 'consumer_name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}), - 'consumer_secret': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32'}), - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'instance_guid': ('django.db.models.fields.CharField', [], {'max_length': '255', 'unique': 'True', 'null': 'True'}) - }, - 'lti_provider.ltiuser': { - 'Meta': {'unique_together': "(('lti_consumer', 'lti_user_id'),)", 'object_name': 'LtiUser'}, - 'edx_user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'unique': 'True'}), - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'lti_consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['lti_provider.LtiConsumer']"}), - 'lti_user_id': ('django.db.models.fields.CharField', [], {'max_length': '255'}) - }, - 'lti_provider.outcomeservice': { - 'Meta': {'object_name': 'OutcomeService'}, - 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), - 'lis_outcome_service_url': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}), - 'lti_consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['lti_provider.LtiConsumer']"}) - } - } - - complete_apps = ['lti_provider'] diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 61c024a107..75ecc2172f 100644 --- a/lms/djangoapps/lti_provider/models.py +++ b/lms/djangoapps/lti_provider/models.py @@ -118,21 +118,3 @@ class GradedAssignment(models.Model): Uniqueness constraints. """ unique_together = ('outcome_service', 'lis_result_sourcedid') - - -class LtiUser(models.Model): - """ - Model mapping the identity of an LTI user to an account on the edX platform. - The LTI user_id field is guaranteed to be unique per LTI consumer (per - to the LTI spec), so we guarantee a unique mapping from LTI to edX account - by using the lti_consumer/lti_user_id tuple. - """ - lti_consumer = models.ForeignKey(LtiConsumer) - lti_user_id = models.CharField(max_length=255) - edx_user = models.ForeignKey(User, unique=True) - - class Meta(object): - """ - Uniqueness constraints. - """ - unique_together = ('lti_consumer', 'lti_user_id') diff --git a/lms/djangoapps/lti_provider/tests/test_users.py b/lms/djangoapps/lti_provider/tests/test_users.py deleted file mode 100644 index 508ab703ac..0000000000 --- a/lms/djangoapps/lti_provider/tests/test_users.py +++ /dev/null @@ -1,157 +0,0 @@ -""" -Tests for the LTI user management functionality -""" - -import string - -from django.contrib.auth.models import User -from django.test import TestCase -from django.test.client import RequestFactory -from mock import patch, MagicMock -from lti_provider.models import LtiConsumer, LtiUser -import lti_provider.users as users -from student.tests.factories import UserFactory - - -class UserManagementHelperTest(TestCase): - """ - Tests for the helper functions in users.py - """ - - def setUp(self): - super(UserManagementHelperTest, self).setUp() - self.request = RequestFactory().post('/') - self.old_user = UserFactory.create() - self.new_user = UserFactory.create() - self.new_user.save() - self.request.user = self.old_user - self.lti_user = LtiUser( - lti_user_id='lti_user_id', - edx_user=self.new_user - ) - - @patch('lti_provider.users.login') - def test_new_user_logged_in_by_switch_user(self, login_mock): - with patch('lti_provider.users.User.objects.get', return_value=self.new_user): - users.switch_user(self.request, self.lti_user) - login_mock.assert_called_with(self.request, self.new_user) - - @patch('lti_provider.users.login') - def test_backend_set_in_switch_user(self, _login_mock): - users.switch_user(self.request, self.lti_user) - self.assertIsNotNone(self.new_user.backend, 'Backend not set on user') - - def test_random_username_generator(self): - for _idx in range(1000): - username = users.generate_random_edx_username() - self.assertTrue(len(username) <= 30, 'Username too long') - # Check that the username contains only allowable characters - for char in range(len(username)): - self.assertTrue( - username[char] in string.ascii_letters + string.digits, - "Username has forbidden character '{}'".format(username[char]) - ) - - -@patch('lti_provider.users.switch_user') -@patch('lti_provider.users.create_lti_user') -class AuthenticateLtiUserTest(TestCase): - """ - Tests for the authenticate_lti_user function in users.py - """ - def setUp(self): - super(AuthenticateLtiUserTest, self).setUp() - self.lti_consumer = LtiConsumer( - consumer_name='TestConsumer', - consumer_key='TestKey', - consumer_secret='TestSecret' - ) - self.lti_consumer.save() - self.lti_user_id = 'lti_user_id' - self.edx_user_id = 'edx_user_id' - self.old_user = UserFactory.create() - self.request = RequestFactory().post('/') - self.request.user = self.old_user - - def create_lti_user_model(self): - """ - Generate and save a User and an LTI user model - """ - edx_user = User(username=self.edx_user_id) - edx_user.save() - lti_user = LtiUser( - lti_consumer=self.lti_consumer, - lti_user_id=self.lti_user_id, - edx_user=edx_user - ) - lti_user.save() - return lti_user - - def test_authentication_with_new_user(self, _create_user, switch_user): - lti_user = MagicMock() - lti_user.edx_user_id = self.edx_user_id - with patch('lti_provider.users.create_lti_user', return_value=lti_user) as create_user: - users.authenticate_lti_user(self.request, self.lti_user_id, self.lti_consumer) - create_user.assert_called_with(self.lti_user_id, self.lti_consumer) - switch_user.assert_called_with(self.request, lti_user) - - def test_authentication_with_authenticated_user(self, create_user, switch_user): - lti_user = self.create_lti_user_model() - self.request.user = lti_user.edx_user - self.request.user.is_authenticated = MagicMock(return_value=True) - users.authenticate_lti_user(self.request, self.lti_user_id, self.lti_consumer) - self.assertFalse(create_user.called) - self.assertFalse(switch_user.called) - - def test_authentication_with_unauthenticated_user(self, create_user, switch_user): - lti_user = self.create_lti_user_model() - self.request.user = lti_user.edx_user - self.request.user.is_authenticated = MagicMock(return_value=False) - users.authenticate_lti_user(self.request, self.lti_user_id, self.lti_consumer) - self.assertFalse(create_user.called) - switch_user.assert_called_with(self.request, lti_user) - - def test_authentication_with_wrong_user(self, create_user, switch_user): - lti_user = self.create_lti_user_model() - self.request.user = self.old_user - self.request.user.is_authenticated = MagicMock(return_value=True) - users.authenticate_lti_user(self.request, self.lti_user_id, self.lti_consumer) - self.assertFalse(create_user.called) - switch_user.assert_called_with(self.request, lti_user) - - -class CreateLtiUserTest(TestCase): - """ - Tests for the create_lti_user function in users.py - """ - - def setUp(self): - super(CreateLtiUserTest, self).setUp() - self.lti_consumer = LtiConsumer( - consumer_name='TestConsumer', - consumer_key='TestKey', - consumer_secret='TestSecret' - ) - self.lti_consumer.save() - - def test_create_lti_user_creates_auth_user_model(self): - users.create_lti_user('lti_user_id', self.lti_consumer) - self.assertEqual(User.objects.count(), 1) - - @patch('uuid.uuid4', return_value='random_uuid') - @patch('lti_provider.users.generate_random_edx_username', return_value='edx_id') - def test_create_lti_user_creates_correct_user(self, uuid_mock, _username_mock): - users.create_lti_user('lti_user_id', self.lti_consumer) - self.assertEqual(User.objects.count(), 1) - user = User.objects.get(username='edx_id') - self.assertEqual(user.email, 'edx_id@lti.example.com') - uuid_mock.assert_called_with() - - @patch('lti_provider.users.generate_random_edx_username', side_effect=['edx_id', 'new_edx_id']) - def test_unique_username_created(self, username_mock): - User(username='edx_id').save() - users.create_lti_user('lti_user_id', self.lti_consumer) - self.assertEqual(username_mock.call_count, 2) - self.assertEqual(User.objects.count(), 2) - user = User.objects.get(username='new_edx_id') - self.assertEqual(user.email, 'new_edx_id@lti.example.com') diff --git a/lms/djangoapps/lti_provider/tests/test_views.py b/lms/djangoapps/lti_provider/tests/test_views.py index f42cab0de3..26e5b1a89b 100644 --- a/lms/djangoapps/lti_provider/tests/test_views.py +++ b/lms/djangoapps/lti_provider/tests/test_views.py @@ -24,7 +24,6 @@ LTI_DEFAULT_PARAMS = { 'oauth_signature_method': u'HMAC-SHA1', 'oauth_timestamp': u'OAuth Timestamp', 'oauth_nonce': u'OAuth Nonce', - 'user_id': u'LTI_User', } LTI_OPTIONAL_PARAMS = { @@ -90,8 +89,7 @@ class LtiLaunchTest(LtiTestMixin, TestCase): Tests for the lti_launch view """ @patch('lti_provider.views.render_courseware') - @patch('lti_provider.views.authenticate_lti_user') - def test_valid_launch(self, _authenticate, render): + def test_valid_launch(self, render): """ Verifies that the LTI launch succeeds when passed a valid request. """ @@ -101,8 +99,7 @@ class LtiLaunchTest(LtiTestMixin, TestCase): @patch('lti_provider.views.render_courseware') @patch('lti_provider.views.store_outcome_parameters') - @patch('lti_provider.views.authenticate_lti_user') - def test_outcome_service_registered(self, _authenticate, store_params, _render): + def test_outcome_service_registered(self, store_params, _render): """ Verifies that the LTI launch succeeds when passed a valid request. """ @@ -145,8 +142,7 @@ class LtiLaunchTest(LtiTestMixin, TestCase): self.assertEqual(response.status_code, 403) @patch('lti_provider.views.lti_run') - @patch('lti_provider.views.authenticate_lti_user') - def test_session_contents_after_launch(self, _authenticate, _run): + def test_session_contents_after_launch(self, _run): """ Verifies that the LTI parameters and the course and usage IDs are properly stored in the session @@ -160,8 +156,7 @@ class LtiLaunchTest(LtiTestMixin, TestCase): self.assertEqual(session[key], request.POST[key], key + ' not set in the session') @patch('lti_provider.views.lti_run') - @patch('lti_provider.views.authenticate_lti_user') - def test_optional_parameters_in_session(self, _authenticate, _run): + def test_optional_parameters_in_session(self, _run): """ Verifies that the outcome-related optional LTI parameters are properly stored in the session @@ -187,6 +182,17 @@ class LtiLaunchTest(LtiTestMixin, TestCase): 'Consumer instance GUID not set in the session' ) + def test_redirect_for_non_authenticated_user(self): + """ + Verifies that if the lti_launch view is called by an unauthenticated + user, the response will redirect to the login page with the correct + URL + """ + request = build_launch_request(False) + response = views.lti_launch(request, unicode(COURSE_KEY), unicode(USAGE_KEY)) + self.assertEqual(response.status_code, 302) + self.assertEqual(response['Location'], '/accounts/login?next=/lti_provider/lti_run') + def test_forbidden_if_signature_fails(self): """ Verifies that the view returns Forbidden if the LTI OAuth signature is @@ -198,21 +204,6 @@ class LtiLaunchTest(LtiTestMixin, TestCase): self.assertEqual(response.status_code, 403) self.assertEqual(response.status_code, 403) - @patch('lti_provider.views.render_courseware') - @patch('lti_provider.views.authenticate_lti_user') - def test_user_authentication_called(self, authenticate, _render): - """ - Verifies that the view returns Forbidden if the LTI OAuth signature is - incorrect. - """ - request = build_launch_request() - views.lti_launch( - request, - unicode(COURSE_PARAMS['course_key']), - unicode(COURSE_PARAMS['usage_key']) - ) - authenticate.assert_called_with(request, u'LTI_User', self.consumer) - class LtiRunTest(LtiTestMixin, TestCase): """ diff --git a/lms/djangoapps/lti_provider/users.py b/lms/djangoapps/lti_provider/users.py deleted file mode 100644 index e6525707fa..0000000000 --- a/lms/djangoapps/lti_provider/users.py +++ /dev/null @@ -1,94 +0,0 @@ -""" -LTI user management functionality. This module reconciles the two identities -that an individual has in the campus LMS platform and on edX. -""" - -import string -import random -import uuid - -from django.contrib.auth import login -from django.contrib.auth.models import User -from django.db import IntegrityError - -from lti_provider.models import LtiUser - - -def authenticate_lti_user(request, lti_user_id, lti_consumer): - """ - Determine whether the user specified by the LTI launch has an existing - account. If not, create a new Django User model and associate it with an - LtiUser object. - - If the currently logged-in user does not match the user specified by the LTI - launch, log out the old user and log in the LTI identity. - """ - try: - lti_user = LtiUser.objects.get( - lti_user_id=lti_user_id, - lti_consumer=lti_consumer - ) - except LtiUser.DoesNotExist: - # This is the first time that the user has been here. Create an account. - lti_user = create_lti_user(lti_user_id, lti_consumer) - - if not (request.user.is_authenticated() and - request.user == lti_user.edx_user): - # The user is not authenticated, or is logged in as somebody else. - # Switch them to the LTI user - switch_user(request, lti_user) - - -def create_lti_user(lti_user_id, lti_consumer): - """ - Generate a new user on the edX platform with a random username and password, - and associates that account with the LTI identity. - """ - edx_password = str(uuid.uuid4()) - - created = False - while not created: - try: - edx_user_id = generate_random_edx_username() - edx_user = User.objects.create_user( - username=edx_user_id, - password=edx_password, - email='{}@lti.example.com'.format(edx_user_id) - ) - edx_user.save() - created = True - except IntegrityError: - # The random edx_user_id wasn't unique. Since 'created' is still - # False, we will retry with a different random ID. - pass - - lti_user = LtiUser( - lti_consumer=lti_consumer, - lti_user_id=lti_user_id, - edx_user=edx_user - ) - lti_user.save() - return lti_user - - -def switch_user(request, lti_user): - """ - Log out the current user, and log in using the edX identity associated with - the LTI ID. - """ - # The login function wants to know what backend authenticated the user. - lti_user.edx_user.backend = 'LTI_Provider' - login(request, lti_user.edx_user) - - -def generate_random_edx_username(): - """ - Create a valid random edX user ID. An ID is at most 30 characters long, and - can contain upper and lowercase letters and numbers. - :return: - """ - allowable_chars = string.ascii_letters + string.digits - username = '' - for _index in range(30): - username = username + random.SystemRandom().choice(allowable_chars) - return username diff --git a/lms/djangoapps/lti_provider/views.py b/lms/djangoapps/lti_provider/views.py index 10b1f1ee0a..454a236252 100644 --- a/lms/djangoapps/lti_provider/views.py +++ b/lms/djangoapps/lti_provider/views.py @@ -4,6 +4,8 @@ LTI Provider view functions from django.conf import settings from django.contrib.auth.decorators import login_required +from django.contrib.auth.views import redirect_to_login +from django.core.urlresolvers import reverse from django.http import HttpResponseBadRequest, HttpResponseForbidden, Http404 from django.views.decorators.csrf import csrf_exempt import logging @@ -11,7 +13,6 @@ import logging from lti_provider.outcomes import store_outcome_parameters from lti_provider.models import LtiConsumer from lti_provider.signature_validator import SignatureValidator -from lti_provider.users import authenticate_lti_user from lms_xblock.runtime import unquote_slashes from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys import InvalidKeyError @@ -23,7 +24,7 @@ log = logging.getLogger("edx.lti_provider") REQUIRED_PARAMETERS = [ 'roles', 'context_id', 'oauth_version', 'oauth_consumer_key', 'oauth_signature', 'oauth_signature_method', 'oauth_timestamp', - 'oauth_nonce', 'user_id' + 'oauth_nonce' ] OPTIONAL_PARAMETERS = [ @@ -91,17 +92,9 @@ def lti_launch(request, course_id, usage_id): params['usage_key'] = usage_key request.session[LTI_SESSION_KEY] = params - try: - lti_consumer = LtiConsumer.get_or_supplement( - params.get('tool_consumer_instance_guid', None), - params['oauth_consumer_key'] - ) - except LtiConsumer.DoesNotExist: - return HttpResponseForbidden() - - # Create an edX account if the user identifed by the LTI launch doesn't have - # one already, and log the edX account into the platform. - authenticate_lti_user(request, params['user_id'], lti_consumer) + if not request.user.is_authenticated(): + run_url = reverse('lti_provider.views.lti_run') + return redirect_to_login(run_url, settings.LOGIN_URL) return lti_run(request)