Merge in changes to allow multiple LTIUser.edx_user_id for different LTIConsumers

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)

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),
+        ),
+    ]

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')

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',))

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):
     """

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()