diff --git a/lms/djangoapps/lti_provider/admin.py b/lms/djangoapps/lti_provider/admin.py index 7776b4821ff2..c3610e9d0a1d 100644 --- a/lms/djangoapps/lti_provider/admin.py +++ b/lms/djangoapps/lti_provider/admin.py @@ -13,4 +13,10 @@ class LtiConsumerAdmin(admin.ModelAdmin): search_fields = ('consumer_name', 'consumer_key', 'instance_guid') list_display = ('id', 'consumer_name', 'consumer_key', 'instance_guid') + def get_readonly_fields(self, request, obj=None): + if obj and obj.pk: + return ("auto_link_users_using_email",) + return super().get_readonly_fields(request, obj) + + admin.site.register(LtiConsumer, LtiConsumerAdmin) diff --git a/lms/djangoapps/lti_provider/migrations/0004_auto_link_users_using_email.py b/lms/djangoapps/lti_provider/migrations/0004_auto_link_users_using_email.py new file mode 100644 index 000000000000..c9b9a346b560 --- /dev/null +++ b/lms/djangoapps/lti_provider/migrations/0004_auto_link_users_using_email.py @@ -0,0 +1,26 @@ +# Generated by Django 3.2.21 on 2023-09-29 07:22 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('lti_provider', '0003_auto_20161118_1040'), + ] + + operations = [ + migrations.AddField( + model_name='lticonsumer', + name='auto_link_users_using_email', + field=models.BooleanField(blank=True, default=False), + ), + migrations.AlterField( + model_name='ltiuser', + name='edx_user', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL), + ), + ] diff --git a/lms/djangoapps/lti_provider/migrations/0004_lticonsumer_auto_link_users_using_email.py b/lms/djangoapps/lti_provider/migrations/0004_lticonsumer_auto_link_users_using_email.py deleted file mode 100644 index 2ea7f88244ce..000000000000 --- a/lms/djangoapps/lti_provider/migrations/0004_lticonsumer_auto_link_users_using_email.py +++ /dev/null @@ -1,18 +0,0 @@ -# Generated by Django 3.2.21 on 2023-09-21 12:18 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('lti_provider', '0003_auto_20161118_1040'), - ] - - operations = [ - migrations.AddField( - model_name='lticonsumer', - name='auto_link_users_using_email', - field=models.BooleanField(blank=True, default=False), - ), - ] diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 93bde45d63bd..ba8fda5c5283 100644 --- a/lms/djangoapps/lti_provider/models.py +++ b/lms/djangoapps/lti_provider/models.py @@ -141,7 +141,7 @@ class LtiUser(models.Model): """ lti_consumer = models.ForeignKey(LtiConsumer, on_delete=models.CASCADE) lti_user_id = models.CharField(max_length=255) - edx_user = models.OneToOneField(User, on_delete=models.CASCADE) + edx_user = models.ForeignKey(User, on_delete=models.CASCADE) class Meta: unique_together = ('lti_consumer', 'lti_user_id') diff --git a/lms/djangoapps/lti_provider/tests/test_admin.py b/lms/djangoapps/lti_provider/tests/test_admin.py new file mode 100644 index 000000000000..7c2d1d18381c --- /dev/null +++ b/lms/djangoapps/lti_provider/tests/test_admin.py @@ -0,0 +1,31 @@ +""" +Tests for the LTI Provider's Admin Views +""" +from unittest.mock import Mock + +from django.contrib.admin.sites import AdminSite +from django.test import TestCase + +from lms.djangoapps.lti_provider.admin import LtiConsumerAdmin +from lms.djangoapps.lti_provider.models import LtiConsumer + + +class LtiConsumerAdminTests(TestCase): + """ + Test the customizations applied for the LtiConsumerAdmin + """ + def setUp(self): + self.site = AdminSite() + self.consumer = LtiConsumer( + consumer_name="Test Consumer", + consumer_key="test-key", + consumer_secret="secret", + ) + self.consumer.save() + + def test_lticonsumeradmin_read_only_fields(self): + ma = LtiConsumerAdmin(LtiConsumer, self.site) + request = Mock() + + self.assertEqual(ma.get_readonly_fields(request, None), ()) + self.assertEqual(ma.get_readonly_fields(request, self.consumer), ('auto_link_users_using_email',)) diff --git a/lms/djangoapps/lti_provider/tests/test_users.py b/lms/djangoapps/lti_provider/tests/test_users.py index 867d4874e27a..d8761658d378 100644 --- a/lms/djangoapps/lti_provider/tests/test_users.py +++ b/lms/djangoapps/lti_provider/tests/test_users.py @@ -9,6 +9,7 @@ import pytest from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import PermissionDenied +from django.db.utils import IntegrityError from django.test import TestCase from django.test.client import RequestFactory @@ -112,7 +113,7 @@ def test_authentication_with_new_user(self, _create_user, switch_user): lti_user.edx_user_id = self.edx_user_id with patch('lms.djangoapps.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, "") + create_user.assert_called_with(self.lti_user_id, self.lti_consumer) switch_user.assert_called_with(self.request, lti_user, self.lti_consumer) def test_authentication_with_authenticated_user(self, create_user, switch_user): @@ -145,7 +146,7 @@ def test_auto_linking_of_users_using_lis_person_contact_email_primary(self, crea request.user = self.old_user users.authenticate_lti_user(request, self.lti_user_id, self.lti_consumer) - create_user.assert_called_with(self.lti_user_id, self.lti_consumer, "") + create_user.assert_called_with(self.lti_user_id, self.lti_consumer) self.lti_consumer.auto_link_users_using_email = True self.lti_consumer.save() @@ -195,6 +196,25 @@ def test_existing_user_is_linked(self): assert lti_user.lti_consumer == self.lti_consumer assert lti_user.edx_user == self.existing_user + def test_only_one_lti_user_edx_user_for_each_lti_consumer(self): + users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) + + with pytest.raises(IntegrityError): + users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) + + def test_create_multiple_lti_users_for_edx_user_if_lti_consumer_varies(self): + lti_consumer_2 = LtiConsumer( + consumer_name="SecondConsumer", + consumer_key="SecondKey", + consumer_secret="SecondSecret", + ) + lti_consumer_2.save() + + lti_user_1 = users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) + lti_user_2 = users.create_lti_user('lti_user_id', lti_consumer_2, self.existing_user.email) + + assert lti_user_1.edx_user == lti_user_2.edx_user + class LtiBackendTest(TestCase): """ diff --git a/lms/djangoapps/lti_provider/users.py b/lms/djangoapps/lti_provider/users.py index 51e5ecc1e70e..fc903aa63a2c 100644 --- a/lms/djangoapps/lti_provider/users.py +++ b/lms/djangoapps/lti_provider/users.py @@ -38,10 +38,11 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer): ) except LtiUser.DoesNotExist: # This is the first time that the user has been here. Create an account. - lis_email = "" if lti_consumer.auto_link_users_using_email: - lis_email = request.POST.get("lis_person_contact_email_primary", "") - lti_user = create_lti_user(lti_user_id, lti_consumer, lis_email) + lis_email = request.POST.get("lis_person_contact_email_primary") + lti_user = create_lti_user(lti_user_id, lti_consumer, lis_email) + else: + lti_user = create_lti_user(lti_user_id, lti_consumer) log.info( "authenticate_lti_user %s", lti_user @@ -54,19 +55,16 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer): switch_user(request, lti_user, lti_consumer) -def create_lti_user(lti_user_id, lti_consumer, email=""): +def create_lti_user(lti_user_id, lti_consumer, email=None): """ 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()) + edx_user = User.objects.filter(email=email).first() if email else None - existing_user = User.objects.filter(email=email).first() if email else None - - if existing_user: - edx_user = existing_user - else: + if not edx_user: created = False + edx_password = str(uuid.uuid4()) while not created: try: edx_user_id = generate_random_edx_username()